fix(dialog): fade-out animation when lazy loaded on popover overlays #4937
Conversation
Branch previewReview the following VRT differencesWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
If the changes are expected, update the |
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
Tachometer resultsChromecontextual-help permalinkbasic-test
dialog permalinkbasic-test
Firefoxcontextual-help permalinkbasic-test
dialog permalinkbasic-test
|
Pull Request Test Coverage Report for Build 11907435888Details
💛 - Coveralls |
| import { alertDestructive } from './dialog.stories.js'; | ||
| import { portrait } from './images.js'; | ||
| import { disabledButtonDecorator } from './index.js'; | ||
|
|
||
| const withOverlayDecorator = (story: () => TemplateResult): TemplateResult => { |
There was a problem hiding this comment.
This decorator needs to be initialized. This will create problems in VRT tests
There was a problem hiding this comment.
Maybe I'm missing something here but I'm not sure what you mean by initializing it. I extracted the function as it was previously defined for all the stories from this file, to be able to add it only where needed.
Is this related to the VRTs saying that this PR removes the disabledButton VRT (link)?
There was a problem hiding this comment.
I see you are trying to add withOverlayDecorator decorator with every story! Instead of adding this as story.decorator = [withOverlayDecorator] you can initialise this as
export default {
....
decorators:[withOverlayDecorator()]
}
But this is not something that will trigger the issue! In the mean time I see some flaky tests have emerged and a redundant hash is there. Can you please updated the hash and surface up the new baseline for slotted and re-trigger the runs.
There was a problem hiding this comment.
That decorator is used with every story except the one that this PR adds, that's why I extracted it from the default export, I don't want it applied in said story. That story is created different to emphasize the lazy loaded usage of the component.
As for the flaky tests, I will update the hash right away.
There was a problem hiding this comment.
OK this is failing coz when you add disabledButton.decorators = [withOverlayDecorator, disabledButtonDecorator], the decorators are not getting ready. Can you try adding them separately instead?
disabledButton.decorators = [withOverlayDecorator];
disabledButton.decorators = [disabledButtonDecorator];OR combining the decorators?
const combinedDecorator = () => {
withOverlayDecorator();
disabledButtonDecorator();
};
disabledButton.decorators = [combinedDecorator];
mizgaionutalexandru
force-pushed
the
imizga/trigger-directive-transition
branch
from
12aa2ad to
10fa1f7
Compare
mizgaionutalexandru
force-pushed
the
imizga/trigger-directive-transition
branch
from
10fa1f7 to
167f7d5
Compare
|
Good work here @mizgaionutalexandru. Let me know if we can land this now? |
…4937) * fix(dialog): fade-out animation when lazy loaded on popover overlays * fix(dialog): missing fade-out animations close dispatch * chore: update storybook paragraph * fix: underlay guard * chore: hash update * chore: golden hash update * ci: updated golden image cache * fix: combined decorators * ci: updated golden image cache --------- Co-authored-by: TaraT <ttomar@adobe.com> Co-authored-by: Rajdeep Chandra <rajdeepchandra@Rajdeeps-MacBook-Pro-2.local> Co-authored-by: Rajdeep Chandra <rajdeepchandra@rajdeeps-mbp-2.macromedia.com>
Description
This PR updates the moment of the
closeevent dispatch from theDialogBaseto happen after all the component is done animating. If the transition duration is 0, the event will dispatch instantly as before.Related issue(s)
Motivation and context
Using a
DialogBaseinside a lazy-loaded overlay content would not always allow for the fade-out animations to complete. Unsure why this issue happens only with the popover overlay, but this fix seems more in line with the documentation stating that the close event "announces that the dialog has been closed".How has this been tested?
Test case 1
Test case 2
Did it pass in Desktop?
Did it pass in Mobile?
Did it pass in iPad?
Screenshots (if appropriate)
Before:
before.mov
After:
after.mov
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.