US152268 - Hookup new vdiff tests and migrate dialog #3892
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
Co-authored-by: github-actions <[email protected]>
There was a problem hiding this comment.
Ok, this is ready for final review! The image diffs here were done in stages:
- Migrate existing images from
/screenshots/ci->golden - Run tests against old images and create useful report to compare
- Merge in changes
But what that ends up looking like all combined is a diff that deletes the old images and adds slightly different new ones. The easiest way to compare the diffs will be to look at the final report in #3893
| @@ -1,4 +1,5 @@ | |||
| * @BrightspaceUI/gaudi-dev | |||
| helpers/getLocalizeResources.js @BrightspaceUI/team-usa-devs @BrightspaceUI/team-usa-senior-devs | |||
| golden/ | |||
| .vdiff.json | |||
There was a problem hiding this comment.
We need to add this so a CODEOWNERS review is not required for vdiff PRs
| @@ -1,6 +1,7 @@ | |||
| build/ | |||
| generated/ | |||
| node_modules/ | |||
| .vdiff/ | |||
| components/*/test/screenshots/current/ | |||
There was a problem hiding this comment.
We can clean these all up after the last old-visual-diff-style tests are migrated
|
|
||
| describe('dialog-fullscreen', () => { | ||
|
|
||
| [/*'native',*/ 'custom'].forEach((type) => { |
There was a problem hiding this comment.
As discussed, we're going to turn off the native tests for now since they're just duplicates of the custom images. But the test file is still setup to properly detect it, and if we want to turn them on again someday we would:
- Uncomment this line and run the tests to get a baseline
- Turn native dialogs back on to see the differences between it and custom
svanherk
changed the title
| { name: 'no-padding', template: createDialog({ content: html`<div style="background-color: var(--d2l-color-citrine); height: 100%; width: 100%;">No padding!</div>${footer}`, noPadding: true }) }, | ||
| { name: 'horizontal-overflow', template: createDialog({ content: html`${tabs}${general}` }) }, | ||
| { name: 'scroll-bottom-shadow', template: createDialog({ content: html`${long}${footer}` }) }, | ||
| { name: 'scroll-top-shadow', template: createDialog({ content: html`${long}${footer}` }), action: elem => elem.querySelector('#bottom').scrollIntoView() }, |
There was a problem hiding this comment.
Was wondering why this resulted in a diff... so previously #dialogLong went to "Line 12" but your ${long} has 13 lines. Same thing happened in the dialog.vdiff.js tests.
There was a problem hiding this comment.
Yeah I should've called that out, I figured it wasn't worth adding another option in dialog-shared-contents.js to export or creating a one-off template just to have one less line
There was a problem hiding this comment.
Cool yeah, just wanted to make sure you did it consciously.
|
🎉 This PR is included in version 2.141.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
ghost
added
the
This PR:
vdiffworkflowdialog,dialog-fullscreen,dialog-mixinanddialog-with-mobile-dropdowntests overdialog-confirmanddialog-ifrauare different enough that there wasn't much benefit lumping them with these four