-
Notifications
You must be signed in to change notification settings - Fork 25
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
Migrate input radio & checkbox vdiffs #4083
Conversation
[false, true].forEach(disabled => { | ||
const checkedStates = ['checked', 'unchecked']; | ||
if (!skeleton) { | ||
checkedStates.push('indeterminate'); |
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 old HTML file actually has fixtures for skeleton + indeterminate but I've excluded them to be consistent. I can add them in though if we think they're valuable.
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.
Interesting, as in had fixtures for them but no actual tests/goldens? It would clean up three lines of code for some (hopefully) simple, non-flaky diffs, but not sure how valuable they are. I'd say I'm indifferent
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.
Yeah exactly. They're not testing anything interesting IMO so I'm going to leave them off.
test/load-sass.js
Outdated
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.
Centralized this since a few of our vdiff tests need it now.
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
await visualDiff.screenshotAndCompare(page, this.test.fullTitle(), { clip: rect }); | ||
}); | ||
if (state !== 'disabled') { | ||
it(`${id}-focus`, async function() { |
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.
Looks like these focus
ones didn't get moved over, was that on purpose?
Edit: Specifically the sass ones, I missed that the others were
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.
Oops yep thanks. Splitting wc
out made it complicated and I forgot about those -- restored them now.
Co-authored-by: github-actions <github-actions@github.com>
da7e6a0
to
1b3b27a
Compare
Co-authored-by: github-actions <github-actions@github.com>
🎉 This PR is included in version 2.149.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Nothing major here, although anything "disabled" because it's using opacity is perceived as a difference. I renamed some of the tests/goldens to not have
wc-
in them.