Update d2l-alert and d2l-alert-toast to use semantic variables.#6728
Update d2l-alert and d2l-alert-toast to use semantic variables.#6728
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
| { name: 'long-button-text-subtext', template: html`<d2l-alert-toast no-auto-close type="warning" button-text="Perform Lengthy Operation Now" open subtext="More detail explaining the critical issue.">A message.</d2l-alert-toast>` } | ||
| ].forEach(({ name, template }) => { | ||
| { name: 'long-button-text-subtext', template: html`<d2l-alert-toast no-auto-close type="warning" button-text="Perform Lengthy Operation Now" open subtext="More detail explaining the critical issue.">A message.</d2l-alert-toast>` }, | ||
| { name: 'long-button-text-subtext-dark', colorMode: 'dark', template: html`<d2l-alert-toast no-auto-close type="warning" button-text="Perform Lengthy Operation Now" open subtext="More detail explaining the critical issue.">A message.</d2l-alert-toast>` } |
There was a problem hiding this comment.
Just added this one test here, which basically covers everything we need for dark mode d2l-alert-toast.
|
I am purposefully not updating the shadow because the floating shadow is very slightly different and will result in a bunch of visual-diffs for toasts. I'd rather wait until shadow decisions are finalized. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
components/alert/test/alert.vdiff.js
Outdated
| { name: 'type-default', template: html`<d2l-alert type="default">A default message.</d2l-alert>` }, | ||
| { name: 'type-default-dark', colorMode: 'dark', template: html`<d2l-alert type="default">A default message.</d2l-alert>` }, | ||
| { name: 'type-success', template: html`<d2l-alert type="success">A success message.</d2l-alert>` }, | ||
| { name: 'type-success-dark', colorMode: 'dark', template: html`<d2l-alert type="success">A success message.</d2l-alert>` }, | ||
| { name: 'type-critical', template: html`<d2l-alert type="critical">A critical message.</d2l-alert>` }, | ||
| { name: 'type-critical-dark', colorMode: 'dark', template: html`<d2l-alert type="critical">A critical message.</d2l-alert>` }, | ||
| { name: 'type-warning', template: html`<d2l-alert type="warning">A warning message.</d2l-alert>` }, | ||
| { name: 'type-warning-dark', colorMode: 'dark', template: html`<d2l-alert type="warning">A warning message.</d2l-alert>` }, | ||
| { name: 'type-error', template: html`<d2l-alert type="error">An error message.</d2l-alert>` }, | ||
| { name: 'type-error-dark', colorMode: 'dark', template: html`<d2l-alert type="error">An error message.</d2l-alert>` }, | ||
| { name: 'type-call-to-action', template: html`<d2l-alert type="call-to-action">A call to action.</d2l-alert>` }, | ||
| { name: 'type-call-to-action-dark', colorMode: 'dark', template: html`<d2l-alert type="call-to-action">A call to action.</d2l-alert>` }, |
There was a problem hiding this comment.
I think this is a good example of what changes if we switch to requesting multiple screenshots in the golden call (like golden({ allColorModes: true }) or whatever). Either it becomes a one word change added to the old test loop above, or the number of new tests here are cut in half.
There was a problem hiding this comment.
Yeah, I think a second it on that for loop would be cleaner, unless I've missed something.
There was a problem hiding this comment.
Yep. I was sort of on the fence about doing this. Although it doesn't make any difference in this case, I've been finding that the loops inside of loops approach to organizing tests is very inflexible. Having said that, I am content to switch it back for alerts. It was mostly out of dislike for that pattern, and also the conditionally constructing the name that would have been required.
The allColorModes idea means that in theory, if we add another color mode, there should be no changes to our test code.
There was a problem hiding this comment.
Yeah, I think a second it on that for loop would be cleaner, unless I've missed something.
Yeah, I considered that briefly too. There's like 5 different ways to do this, and something good & bad can be said about each.
There was a problem hiding this comment.
Ok, I've switched the other way (with repeated test instead of nested loop).
There was a problem hiding this comment.
The allColorModes idea means that in theory, if we add another color mode, there should be no changes to our test code.
Yeah that was my thinking. In theory, you'd only do the work of picking which tests need a second, alternative color mode screenshot once. If a new colour mode is added, you should have full test coverage from the get-go. I think this is still up for debate though, whether we'd want golden({ alts: ['allColorModes', 'rtl'] }) or more fexibility like golden({ alts: ['dark', 'vampire', 'rtl'] }). Also, do we want dark + RTL at the same time?
Anyways, looking at @GZolla's refactor PR in testing now and we're gonna try to get this in early next week. We could all meet to discuss the above.
There was a problem hiding this comment.
Also, do we want dark + RTL at the same time?
That's a good question as it informs the golden(...) API, or however it is done. I'm struggling to think of concrete examples where I would want different color modes * RTL, but I suppose it might come up.
…ore into dbatiste/alert-dark-mode
3 participants
GAUD-9453
This PR:
d2l-alertto use semantic variablesd2l-alert-toastto use semantic variables