-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[test] Migrate regressions to emotion #27010
[test] Migrate regressions to emotion #27010
Conversation
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.
We need no visual changes in Argos :)
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
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.
Let's not use too much API surface. Especially most of the sx
stuff can be removed. It's unrelated to the test and just frustrating to deal with if it breaks.
@eps1lon I think that we have tried to keep the CSS to its minimum in the visual regression tests so far, but there is likely more room for simplification 👍. I would add a note of caution: we added some CSS to create the right environment to reproduce bugs, these will still need to be here. |
Hiding more stuff behind an abstraction (e.g. |
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.
The style
-> sx
changes should be reverted. sx
is unrelated to the tests.
I think it is not clear to me. Should we remove -<Input sx={{ margin: 1 }} />
+<Input style={{ margin: 1 }} /> or only for Box components? -<Box sx={{ margin: 1 }} />
+<div style={{ margin: 1 }} /> |
It's not about removing. It's about keeping it the way it is. We just want to migrate the parts that are using |
What is missing from this? |
Using |
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 for sticking with it. Much appreciated.
One of #16947