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.

Sign up for GitHub

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] Remove top-level inline-block from the regression tests #43656

Merged
merged 7 commits into from
Sep 13, 2024
Merged

[test] Remove top-level inline-block from the regression tests #43656

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
08e35d0
Remove top-level `inline-block`
Janpot Sep 8, 2024
497087c
Update TestViewer.js
Janpot Sep 8, 2024
bbd400f
mimic demo host better
Janpot Sep 12, 2024
c5e007e
change back to block
Janpot Sep 13, 2024
3d76901
Update pnpm-lock.yaml
Janpot Sep 13, 2024
ae345c1
Merge remote-tracking branch 'upstream/master' into deflake-test
Janpot Sep 13, 2024
a68ac62
Update pnpm-lock.yaml
Janpot Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions test/regressions/TestViewer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ function TestViewer(props) {
};
}, []);

const viewerBoxSx = {
display: 'block',
p: 1,
position: 'relative',
Copy link
Member

@oliviertassinari oliviertassinari Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we remove this one?

Suggested change
position: 'relative',

I think we want to be able to catch cases where demos relies on a special parent position properly to be set. When developers copy and past a demo, they won't have this style set.

Copy link
Member

Choose a reason for hiding this comment

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

I'll open a PR for this cc @Janpot

Copy link
Member

Choose a reason for hiding this comment

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

#43743

};

return (
<React.Fragment>
<GlobalStyles
Expand Down Expand Up @@ -77,7 +83,7 @@ function TestViewer(props) {
<JoyBox
aria-busy={!ready}
data-testid="testcase"
sx={{ bgcolor: 'background.body', display: 'inline-block', p: 1 }}
sx={{ bgcolor: 'background.body', ...viewerBoxSx }}
Copy link
Member

@oliviertassinari oliviertassinari Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would this be enough?

Suggested change
sx={{ bgcolor: 'background.body', ...viewerBoxSx }}
sx={{ bgcolor: 'background.body', display: 'block', p: 1 }}

It still makes the screenshots to take more space, so the diff might be slower, but we pay per screenshots now, so we don't have to care so much 😄

Copy link
Member Author

@Janpot Janpot Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

would this be enough?

Yes, it has the same effect. I'm indifferent to the exact solution, as long as it's not inline 🙂 My first commit actually removed the property, which would be the same thing.

Copy link
Member

Choose a reason for hiding this comment

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

Are we ok with docs demos potentially looking slightly different to their corresponding regression screenshots? If that's ok block is fine. Probably the demo wrapper should wrap the contents in a <div> so the demo contents are not affected by flex, although flex can be a good thing to help build more robust components e.g. https://app.argos-ci.com/mui/material-ui/builds/31842/107814095 breaks because it has several top-level elements instead of just one.

Copy link
Member Author

@Janpot Janpot Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Are we ok with docs demos potentially looking slightly different to their corresponding regression screenshots?

There's a nuance here that not all screenshots are originating from docs demos. Some come from the templates. And I believe there's a set of separately curated ones as well. This is what made me open this PR in the first place as the templates behave flaky when rendered in an inline container. I think in light of that, block makes sense. I'm personally ok either way, but let's not waste too much time on this, the idea is to get away from inline-block, the difference between flex and block is far from as impactful as that.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, block then sounds like the best choice.

>
{children}
</JoyBox>
Expand All @@ -86,7 +92,7 @@ function TestViewer(props) {
<Box
aria-busy={!ready}
data-testid="testcase"
sx={{ bgcolor: 'background.default', display: 'inline-block', p: 1 }}
sx={{ bgcolor: 'background.default', ...viewerBoxSx }}
>
{children}
</Box>
Expand Down