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][docs] Improve demos for better regression screenshots #43742

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

aarongarciah
Copy link
Member

@aarongarciah aarongarciah commented Sep 13, 2024

Follow up on #43656 (comment)

Some demos were using flex instead of inline-flex in their top-level elements, causing them to look off when rendered inside a block element instead of a flex element. For context, our demo wrapper uses flex but our visual regression wrapper uses block since #43656.

Before: https://deploy-preview-43740--material-ui.netlify.app/material-ui/react-divider/#flex-item
After: https://deploy-preview-43742--material-ui.netlify.app/material-ui/react-divider/#flex-item

@aarongarciah aarongarciah added docs Improvements or additions to the documentation test labels Sep 13, 2024
@aarongarciah aarongarciah self-assigned this Sep 13, 2024
@mui-bot
Copy link

mui-bot commented Sep 13, 2024

Netlify deploy preview

https://deploy-preview-43742--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 1ca843b

@aarongarciah
Copy link
Member Author

This one is not fixed in this PR: https://app.argos-ci.com/mui/material-ui/builds/32051/108773834. It was broken before #43656 and it looks like a CSS order issue. See attached screenshots and notice the style order in the dev tools.

Regression test
Screenshot 2024-09-13 at 11 51 02

Docs
Screenshot 2024-09-13 at 11 51 22

@@ -19,6 +19,7 @@ export default function ExampleAlignmentButtons() {
onChange={(event: React.ChangeEvent<HTMLInputElement>) =>
setAlignment(event.target.value)
}
sx={{ display: 'inline-flex' }}
Copy link
Member Author

@aarongarciah aarongarciah Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ideal to override the display property of the RadioGroup component in our own demos (RadioGroup uses flex). In hindsight, RadioGroup should've probably used inline-flex from the start.

Another option is to wrap the entire demo in an inline-block/flex element, but that probably adds more noise to the demo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps something we can introduce as a breaking change in a future major?

@aarongarciah
Copy link
Member Author

aarongarciah commented Sep 13, 2024

The 3 demos being updated look good in Argos:

There are 2 other unrelated screenshots that changed:

@aarongarciah aarongarciah marked this pull request as ready for review September 13, 2024 10:50
@Janpot
Copy link
Member

Janpot commented Sep 13, 2024

There are 2 other unrelated screenshots that changed:

Taking a look at those in

@aarongarciah
Copy link
Member Author

Taking a look at those in

In that case I think we can move forward with this PR.

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👌

@aarongarciah aarongarciah merged commit a487633 into mui:master Sep 13, 2024
22 checks passed
@aarongarciah aarongarciah deleted the fix-some-demos-layouts branch September 13, 2024 15:42
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: divider This is the name of the generic UI component, not the React module! labels Sep 13, 2024
@oliviertassinari
Copy link
Member

Nice 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: divider This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants