Skip to content
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

Merged
merged 9 commits into from
Jul 12, 2021

Conversation

vicasas
Copy link
Member

@vicasas vicasas commented Jun 28, 2021

One of #16947

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 28, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 0d2d95f

@vicasas vicasas added the test label Jun 28, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a 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 :)

test/regressions/fixtures/Select/SelectChips.js Outdated Show resolved Hide resolved
test/regressions/fixtures/Textarea/Textarea.js Outdated Show resolved Hide resolved
Copy link
Member

@eps1lon eps1lon left a 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.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 29, 2021

@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.

@eps1lon
Copy link
Member

eps1lon commented Jun 29, 2021

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 👍.

Hiding more stuff behind an abstraction (e.g. sx or Box) does not make a test simpler.

Copy link
Member

@eps1lon eps1lon left a 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.

@vicasas
Copy link
Member Author

vicasas commented Jul 1, 2021

The style -> sx changes should be reverted. sx is unrelated to the tests.

I think it is not clear to me. Should we remove sx and use style in all tests including components being tested

-<Input sx={{ margin: 1 }} />
+<Input style={{ margin: 1 }} />

or only for Box components?

-<Box sx={{ margin: 1 }} />
+<div style={{ margin: 1 }} />

@eps1lon
Copy link
Member

eps1lon commented Jul 1, 2021

I think it is not clear to me. Should we remove sx and use style in all tests including components being tested

It's not about removing. It's about keeping it the way it is. We just want to migrate the parts that are using @material-ui/styles. All the rest should stay that way (e.g. style prop usage).

@vicasas vicasas requested a review from eps1lon July 1, 2021 23:33
@vicasas
Copy link
Member Author

vicasas commented Jul 6, 2021

What is missing from this?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 8, 2021

Using style over sx in the visual regression tests could make sense. It will be less refactoring in the future if we change the API. The style API is more reliable (assuming a 10 years window)

@oliviertassinari oliviertassinari removed their request for review July 8, 2021 00:40
Copy link
Member

@eps1lon eps1lon left a 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.

@eps1lon eps1lon merged commit 108170c into mui:next Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants