-
Notifications
You must be signed in to change notification settings - Fork 215
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
Fix logo height #3967
Fix logo height #3967
Conversation
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 maxDiffPixelRatio
of 0.01 does not catch the small changes in the header. We can lower it to 0.005 to prevent extreme flakiness while also catching differences in the logos.
My request for change is to lower the ratio to 0.005 and update the snapshots:
maxDiffPixelRatio: 0.01, |
I think you will also need to rebase onto main because there were some updates to snapshots in recently-merged PRs.
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.
Everything looks correct 🚀
@obulat of course, the maxDiffPixelRatio! Thank you, I will play with this a little. |
This one is going to take a little bit of time and effort to dial in the tests and identify any existing flake with the pixel ratio. |
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.
Only 93 files 🎉 Great that no other snapshots had to be updated.
Would be nice to make sure that this PR is up-to-date before merging to avoid test regressions.
Fixes
Fixes #476 by @fcoveram.
Description
Fixes the logo by reducing its size and making its size universal. Also removes "baked in" padding to the logo loader; this instead comes from the context of the loader.
The Openverse logo should be the same size everywhere, now.
This PR also removes the global
maxDiffPixelRatio
from our Playwright tests. These tests are fortunately much more stable these days, and removing this gives us better assurances we are capturing changes to our front-end.For example, with that setting enabled, our tests missed the fact that the "Terms" link was added to the header! Some snapshot changes only reflect the change in this link and not a change to the logo.
Testing Instructions
Run the PR locally with
just frontend
. The Openverse logo should be the same size everywhere, now. Click through the site and confirm. Tests should pass too.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin