[test] Remove top-level inline-block from the regression tests#43656
[test] Remove top-level inline-block from the regression tests#43656Janpot merged 7 commits intomui:masterfrom
Conversation
Netlify deploy previewhttps://deploy-preview-43656--material-ui.netlify.app/ Bundle size report |
The intent to do Fun fact: we use opposite |
| aria-busy={!ready} | ||
| data-testid="testcase" | ||
| sx={{ bgcolor: 'background.body', display: 'inline-block', p: 1 }} | ||
| sx={{ bgcolor: 'background.body', ...viewerBoxSx }} |
There was a problem hiding this comment.
would this be enough?
| 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 😄
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, block then sounds like the best choice.
oliviertassinari
added
the
scope: code-infra
oliviertassinari
changed the title
There was a problem hiding this comment.
Looks good! I'm fine with flex or block. flex causes some tests to change because they have several top-level elements, but ideally those components should be "fixed" to be able to render just fine inside flex containers e.g. https://app.argos-ci.com/mui/material-ui/builds/31842/107814095
edit: block seems better since we not only do regression tests of demos but templates and other stuff as well.
github-actions
bot
added
the
PR: out-of-date
github-actions
bot
removed
the
PR: out-of-date
|
Did a quick rundowm, the following ones look off. But we should probably fix them at the demo level: |
|
@Janpot I'll fix those demos in another PR. |
There was a problem hiding this comment.
ideally those components should be "fixed" to be able to render just fine inside flex containers e.g. https://app.argos-ci.com/mui/material-ui/builds/31842/107814095
@aarongarciah Agree but this screenshot is not a valid one. It's part of a template, only the top level template screenshot should be taken, so this one should be skipped, it's redundant with: https://app.argos-ci.com/mui/material-ui/builds/31842/107814093.
| const viewerBoxSx = { | ||
| display: 'block', | ||
| p: 1, | ||
| position: 'relative', |
There was a problem hiding this comment.
Can we remove this one?
| 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.
4 participants
In an effort to solve mui/mui-x#13477 (comment) and the flaky dashboard screenshot.
We're seeing glitches in the screenshot of the dashboard template. It looks like it's caused by being nested it in a
inline-blockcontainer (the docs demo host hasflex).I manually went through all 240 mismatches, I don't see much out of the ordinary other than that most now size to their parent instead of their content. Would argue that real world scenarios are much more likely to be in this situation.
It even captures more information in many of them:
Things to investigate:
All-in-all, I believe this simple change is a step in the good direction. Would definitely encourage anyone with an hour to spare to manually review our visual tests from time to time.