-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PandaCSS - convert thought, thought-annotation, editable #2376
PandaCSS - convert thought, thought-annotation, editable #2376
Conversation
Looks like the Puppeteer snapshot tests are failing. Can you take a look? You can check the diff in the "Upload snapshot diff artifact" workflow step. |
dc2937c
to
b31a1b2
Compare
@raineorshine I've fixed it now, thanks! |
708b6d5
to
cf06186
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This is a big one 😅. I'm not able to test each change individually, but it appears you've been meticulous in converting each complex descendant selector. Hoping for the best! 🤞
@@ -27,7 +27,6 @@ const Toggle = ({ children, expand, title }: { children?: any; expand?: boolean; | |||
> | |||
<g> | |||
<path | |||
className='glyph-fg triangle' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does the ErrorBoundaryContainer
toggle get styled now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it was never styled before, I inspected it, and the classes for glyph-fg and triangle weren't applied, since all the styles were descendant selectors that didn't apply here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, thanks! That's very strange, as I did a git blame and it never seemed to be connected even when it was first created 🤷♀️
For dead code that is removed, if you're able to call that out in the PR for me that would be great. Will save me time on the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! I'll let you know next time.
eb68630
to
26c8cf1
Compare
Sorry the Thought commit turned out so large. There were a lot of descendant selectors involved, and I could have done a better job of splitting things up better. |
That's okay. The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you 🙂
) : ( | ||
<Link className='extend-tap-small' simplePath={simplePath} label={label} /> | ||
))} | ||
{!isDeleting && <Superscript simplePath={simplePath} />} | ||
{!isDeleting && ( | ||
<Superscript simplePath={simplePath} css={{ position: 'relative', left: '-2px', top: '-3px' }} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing an inline array or object literal as a prop results in a different ref every render, preventing the component from being memoized. We need to ensure each prop has a stable object reference if the value does not change.
In this case since the styles are completely static, you can define them in a const
at the module level. This unfortunately moves them further from where they are used, and forces us to have an extra variable, but there is no other way to stabilize the object reference when passing in the css
object.
Note that className
does not have this problem because it is a string
and strings are always compared by value.
className={css({ position: 'static' })} | ||
color='gray' | ||
size={16} | ||
iconStyle={{ position: 'relative', left: -1, top: 2 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also has the object reference problem, although it's not as significant since HomeLink
is rendered rarely, and is a very light component.
26c8cf1
to
ee27d30
Compare
@@ -110,6 +111,7 @@ const BreadCrumb = React.memo( | |||
verticalAlign: 1, | |||
userSelect: 'none', | |||
} | |||
const superscriptCss = { position: 'relative', left: '-2px', top: '-3px' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this will create a new object reference every call to render. You have to move it to the module scope at the top of the file :).
#2170