Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bug: calcite-alert stays open beyond animation end when another alert is added #3366

Closed
2 tasks done
noahmulfinger opened this issue Oct 27, 2021 · 4 comments
Closed
2 tasks done

Bug: calcite-alert stays open beyond animation end when another alert is added #3366

noahmulfinger opened this issue Oct 27, 2021 · 4 comments
Assignees
@benelan
Labels
4 - verified Issues that have been released and confirmed resolved. bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Milestone
Sprint 11/22 - 12/3

Comments

@noahmulfinger
Copy link
Contributor

Actual Behavior

When an auto-dismiss calcite alert is open and second alert is is added, there seems to be a delay between the progress bar animation ending and the first alert closing. You can see in this video:

Screen.Recording.2021-10-27.at.4.03.12.PM.mov

Expected Behavior

I would expect the alert close behavior to be the same regardless of when a new alert has been added.

Reproduction Steps and Sample

Sample:

  1. Go to https://codepen.io/noahmulfinger/pen/rNzwLRK?editors=1010
  2. Click on the Add alerts button and you will see the same behavior as in the above video.

Relevant Info

Reproducible version: @esri/[email protected]

  • CDN
  • NPM package
@noahmulfinger noahmulfinger added bug Bug reports for broken functionality. Issues should include a reproduction of the bug. 0 - new New issues that need assignment. needs triage Planning workflow - pending design/dev review. labels Oct 27, 2021
@benelan benelan removed the needs triage Planning workflow - pending design/dev review. label Nov 3, 2021
@benelan benelan added this to the Sprint 11/8 - 11/19 milestone Nov 3, 2021
@y0n4 y0n4 self-assigned this Nov 15, 2021
@y0n4 y0n4 added 1 - assigned Issues that are assigned to a sprint and a team member. and removed 0 - new New issues that need assignment. labels Nov 15, 2021
y0n4 pushed a commit that referenced this issue Nov 16, 2021
fix(alert): remove clearTimeOUt that delays the alert to close on aut…
1dce970 
…oDismiss mode #3366
@y0n4 y0n4 mentioned this issue Nov 16, 2021
Merged
@y0n4 y0n4 added 2 - in development Issues that are actively being worked on. and removed 1 - assigned Issues that are assigned to a sprint and a team member. labels Nov 23, 2021
y0n4 pushed a commit that referenced this issue Nov 29, 2021
fix(alert): remove clearTimeOut that delays the alert to close on aut…
9376b52 
…oDismiss mode #3366 (#3525)

* fix(alert): remove clearTimeOUt that delays the alert to close on autoDismiss mode #3366

* isolate only 1 timeout id set per each alert, considering utilizing component id to track timeout id

* add conditions to fire setTimeOut one at a time and clean up code

* remove id prop, scrapping id prop idea

* remove comment

* clear time out if there is an existing timeout id

* clear timer id when component closes

* add test for alert component autodismiss duration check

* revise condition from pr comment
@y0n4 y0n4 added 3 - installed Issues that have been merged to master branch and are ready for final confirmation. and removed 2 - in development Issues that are actively being worked on. labels Nov 29, 2021
@github-actions github-actions bot assigned benelan and unassigned y0n4 Nov 29, 2021
@github-actions
Copy link
Contributor

Installed and assigned for verification.

@benelan
Copy link
Member

benelan commented Dec 2, 2021 •
edited
Loading

@macandcheese can you take a look at this one for design? I don't think there should be a close animation if there is another alert already queued. There was no close animation before this fix, we just needed to remove the delay before the progress bar started moving. I used Noah's sample and changed the version to next.

Edit: Never mind there was an animation before this install. However I still think having the animations back to back on next looks weird. So maybe we should remove the animation if there is another alert queued up, thoughts Adam?

@macandcheese
Copy link
Contributor

So, this is kind of a weird one. Because an individual alert animates in, the alert "object" should animate out before another one can replace it. If we didn't do that, we'd be replacing the content of the alert vs. replacing the alert itself, which the animation infers.

That said - it's not a great interaction. Ideally when we handle multiple queued alerts with this update: #2835, we can have queued alerts animate in "behind" and then the animation for the queued alert becoming active will be more of a "move to front", with the dismissed alert animating out.

I realize that probably doesn't make sense, the image in the linked issue may help. For now I think we can leave it until that's implemented.

@benelan
Copy link
Member

benelan commented Dec 2, 2021

Thanks for looking Adam. This is verified on next then.

@benelan benelan closed this as completed Dec 2, 2021
@benelan benelan added 4 - verified Issues that have been released and confirmed resolved. and removed 3 - installed Issues that have been merged to master branch and are ready for final confirmation. labels Dec 2, 2021
@y0n4 y0n4 mentioned this issue Dec 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@benelan benelan

Labels
4 - verified Issues that have been released and confirmed resolved. bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Milestone
Sprint 11/22 - 12/3
Development

No branches or pull requests
4 participants
@macandcheese @noahmulfinger @benelan @y0n4