-
Notifications
You must be signed in to change notification settings - Fork 197
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
Add dark snapshots to storybook tests #4928
Conversation
8d08737
to
3becb32
Compare
845b02d
to
984d05f
Compare
3becb32
to
18dc47e
Compare
d627e9b
to
813db9f
Compare
82e3c7a
to
6bcc141
Compare
813db9f
to
afe26ab
Compare
bf04a21
to
27dc277
Compare
afe26ab
to
b9b6dd3
Compare
bd11e2e
to
effb9d2
Compare
b9b6dd3
to
587a7cd
Compare
07def70
to
14069a0
Compare
ec33781
to
6bf0412
Compare
60c51ae
to
394a116
Compare
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.
LGTM except for the two sleeps. Left a comment asking for clarification.
// Wait for the theme to change. | ||
await sleep(100) | ||
|
||
// Wait for the theme to change. | ||
await sleep(100) |
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.
Is it intentional to have two sleeps? It's unfortunate to need to use one at all... could we instead do something in turnOnDarkMode
that probes the page for the expected classname on the root element?
If we absolutely need two separate sleeps, then it would be better to document them explicitly in reference to each other (and maybe move them into turnOnDarkMode
, if it's always needed anyway).
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 two sleeps are caused by the faulty merge conflict resolution :( I'm removing the extra one.
The existence of class name does not guarantee that the rendering has caught up and all colors have changed. I've seen a range of white-to-gray colors in the separator lines in the flaky snapshots when working on this, so I think a sleep is the best solution here.
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.
Okay, understood.
I'm surprised it doesn't happen in a single rendering pass 🤔. What causes the browser to take multiple passes to render in response to the classname change, I wonder.
1fdba95
to
af8ed47
Compare
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.
LGTM, it makes sense that fixing component issues it outside the scope of this PR. Just because this was a good point at which to review them (all in one PR, easy to find) I looked through the snapshots to see if anything should be noted as needing to fix later. Nothing obviously wrong is there, but perhaps @zackkrida or @dhruvkb are better suited to at least glance through them to make sure. The goal being just to note any issues so we can fix them later.
In any case, it doesn't block merging this PR, review of the snapshots can happen afterwards 🙂
af8ed47
to
10b4c60
Compare
Fixes
Related to #4305 by @zackkrida
Description
This PR adds the dark snapshots to the Storybook tests.
It also adds a 100ms wait after taking the light snapshot, before taking the dark mode snapshot, to make sure that the dark theme has fully rendered (otherwise, the white line underneath the filters tabs was saved as anywhere between gray and white, depending on how much of the theme switching was finished by the time of the snapshot).
The
"slim-filled-borderless"
focus test was removed since we no longer use this class.Testing Instructions
Look through the added snapshots. Only dark mode storybook snapshots should be added. Some elements might need fixing, but not in scope of this PR. The CI should pass.
Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin