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

fix(alert): auto-dismissible retains close button and dismisses timer while a user is hovering over #5872

Merged
merged 21 commits into from
Dec 8, 2022
Merged

fix(alert): auto-dismissible retains close button and dismisses timer while a user is hovering over #5872

merged 21 commits into from
Dec 8, 2022

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Dec 1, 2022

Related Issue: #3338

Summary

If an alert is auto-dismissible,

  • keep the close button there, a user may want to dismiss the alert before the dismiss duration completes.

  • pause the dismiss timer while a user is hovering over, or focusing on an element within an alert. Although disincentivized, apps may put actions or links inside of an auto-dismissible alert.

@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Dec 1, 2022
Elijbet added 13 commits December 5, 2022 14:32
@Elijbet
Merge branch 'master' into elijbet/3338-alert-usability-enhancements-…
3d1bce6 
…to-auto-dismiss
@Elijbet
Merge branch 'master' into elijbet/3338-alert-usability-enhancements-…
e941c0f 
…to-auto-dismiss
@Elijbet
mouseOver and mouseLeave do not need to run if autoDuration is set false
b40d27d
@Elijbet
fix timer and cleanup
9b539c0
@Elijbet
adjust existing related test for correct output: expect close icon to…
cfd4c15 
… be present when auto-dismiss is true
@Elijbet
Merge branch 'master' into elijbet/3338-alert-usability-enhancements-…
f37bfe0 
…to-auto-dismiss
@Elijbet
WIP e2e tests
ea66b78
@Elijbet
master merge
8060b8b
@Elijbet
Merge branch 'master' into elijbet/3338-alert-usability-enhancements-…
ead889d 
…to-auto-dismiss
@Elijbet
add tests for auto-dismiss behavior: should render close button and p…
f0e8c5e 
…auses on mouseOver and resumes on mouseLeave
@Elijbet
Merge branch 'master' into elijbet/3338-alert-usability-enhancements-…
3a870fc 
…to-auto-dismiss
@Elijbet
Merge branch 'master' into elijbet/3338-alert-usability-enhancements-…
bdc9b6a 
…to-auto-dismiss
@Elijbet
added test for auto-dismiss behavior on queued items: should display …
c0f59d1 
…number of queued alerts with a calcite-chip
@Elijbet Elijbet added the enhancement Issues tied to a new feature or request. label Dec 8, 2022
@Elijbet Elijbet marked this pull request as ready for review December 8, 2022 21:20
@Elijbet Elijbet requested a review from a team as a code owner December 8, 2022 21:20
driskull
driskull requested changes Dec 8, 2022
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

src/components/alert/alert.stories.ts Outdated Show resolved Hide resolved
src/components/alert/alert.tsx Outdated Show resolved Hide resolved
src/components/alert/alert.tsx Show resolved Hide resolved
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Dec 8, 2022
driskull
driskull approved these changes Dec 8, 2022
View reviewed changes
Copy link
Member

@driskull driskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@Elijbet Elijbet merged commit 274b104 into master Dec 8, 2022
@Elijbet Elijbet deleted the elijbet/3338-alert-usability-enhancements-to-auto-dismiss branch December 8, 2022 23:32
@github-actions github-actions bot added this to the 2023 January Priorities milestone Dec 8, 2022
@macandcheese
Copy link
Contributor

Extremely awesome work @Elijbet !! The only thing I notice is that while it pauses while hovering, it's not pausing when focus is inside the alert and the cursor is not. For instance, if there is a slotted calcite-link that has focus, the timer should pause as well. Maybe we can just add this as a follow up for later?

@Elijbet
Copy link
Contributor Author

Elijbet commented Dec 9, 2022

Extremely awesome work @Elijbet !! The only thing I notice is that while it pauses while hovering, it's not pausing when focus is inside the alert and the cursor is not. For instance, if there is a slotted calcite-link that has focus, the timer should pause as well. Maybe we can just add this as a follow up for later?

Yeah, that's a good point, I don't think I thought of that. Lets log it as an issue and I'll address that as well.

@Elijbet Elijbet mentioned this pull request Dec 9, 2022
Closed
benelan added a commit that referenced this pull request Dec 12, 2022
@benelan
Merge branch 'master' into benelan/readme-add-badges
a468648 
* master: (55 commits)
  build: update browserslist db (#5986)
  docs: update component READMEs (#5980)
  1.0.0-next.670
  refactor(input-date-picker,date-picker)!: Removing deprecated locale properties (#5977)
  1.0.0-next.669
  refactor(input-date-picker)!: Remove deprecated active property (#5974)
  1.0.0-next.668
  fix(modal, popover): Add `disableFocusTrap` property to toggle focus trapping. (#5965)
  1.0.0-next.667
  refactor(input-time-picker)!: Remove deprecated active property (#5970)
  docs(changelog): fix breaking change indent levels (#5973)
  1.0.0-next.666
  refactor(time-picker)!: Remove deprecated locale property (#5962)
  1.0.0-next.665
  fix(input, input-number, input-text): Fix infinite loop crashing browser. #5882 (#5961)
  test(floating-ui): fix type errors (#5966)
  docs: update component READMEs (#5964)
  1.0.0-next.664
  fix(alert): auto-dismissible retains close button and dismisses timer while a user is hovering over (#5872)
  chore(color-picker): add opacity string (#5959)
  ...
@rebollin rebollin mentioned this pull request Jan 3, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driskull driskull driskull approved these changes

@eriklharper eriklharper Awaiting requested review from eriklharper

Assignees

@Elijbet Elijbet

Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Milestone
2023 January Priorities
Development

Successfully merging this pull request may close these issues.
3 participants
@Elijbet @macandcheese @driskull