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.

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

feat(pie-toast): DSW-2204 implement auto-dismiss / persistent feature #1862

Merged
merged 26 commits into from
Oct 3, 2024

Conversation

thejfreitas
Copy link
Member

@thejfreitas thejfreitas commented Sep 13, 2024

In this Pull Request we are implementing the auto-dismiss feature after 5 seconds or when configurable via duration property and deactivating it if the duration property is null as well persistence when the user hover the mouse onto the component and when the user focus on the action button and close button using the keyboard avoiding closing the toast and reseting the auto-dismiss functionality.

Extra

We've created additional documentation pages for each variant as well its combination with isStrong property.

Screenshots and Videos

Screen.Recording.2024-09-13.at.2.40.03.PM.mov
Screenshot 2024-09-13 at 2 39 29 PM

═══════════════════════════════════════════════════════════

Describe your changes (can list changeset entries if preferable)

Author Checklist (complete before requesting a review)

  • I have performed a self-review of my code
  • I have added thorough tests where applicable (unit / component / visual)
  • I have reviewed the PIE Storybook/PIE Docs PR preview
  • I have reviewed visual test updates properly before approving
  • If changes will affect consumers of the package, I have created a changeset entry.
  • If a changeset file has been created, I have used the /snapit functionality to test my changes in a consuming application

Reviewer checklists (complete before approving)

Reviewer 1 - @jamieomaguire

  • I have reviewed the PIE Storybook/PIE Docs PR preview
  • If there are visual test updates, I have reviewed them

Reviewer 2 - @xander-marjoram

  • I have reviewed the PIE Storybook/PIE Docs PR preview
  • If there are visual test updates, I have reviewed them

@thejfreitas thejfreitas self-assigned this Sep 13, 2024
@thejfreitas thejfreitas requested review from a team as code owners September 13, 2024 20:57
Copy link

changeset-bot bot commented Sep 13, 2024

🦋 Changeset detected

Latest commit: d1c248b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@justeattakeaway/pie-toast Minor
pie-storybook Minor
@justeattakeaway/pie-webc Patch
wc-angular12 Patch
wc-next10 Patch
wc-next13 Patch
wc-nuxt2 Patch
wc-nuxt3 Patch
wc-react17 Patch
wc-react18 Patch
wc-vanilla Patch
wc-vue3 Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

[Lint PR Title] ❌ Failed.
The PR title should follow the same convention used for commits, e.g.:
type(scope): DSW-123 title where DSW-123 is the Jira ticket id or DSW-000 when one is not available.

@thejfreitas thejfreitas changed the title feat(pie-toast): DSW-2204: implement auto-dismiss / persistent feature feat(pie-toast): DSW-2204 implement auto-dismiss / persistent feature Sep 13, 2024
@github-actions github-actions bot dismissed their stale review September 13, 2024 20:58

[Lint PR Title] ✅ All good!

Copy link
Contributor

@kevinrodrigues kevinrodrigues left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 We may need some component tests for some of the behaviour?

packages/components/pie-toast/src/defs.ts Outdated Show resolved Hide resolved
packages/components/pie-toast/src/defs.ts Outdated Show resolved Hide resolved
packages/components/pie-toast/src/index.ts Outdated Show resolved Hide resolved
kevinrodrigues
kevinrodrigues previously approved these changes Sep 30, 2024
@thejfreitas
Copy link
Member Author

A couple of comments about the storybook preview, I'm happy if you want to address these in a follow-up PR:

  1. Please can you use UseArgs to link the value of isOpen to the current state of the toast? We use it in a few components to create a two-way binding between the control and its value (e.g., checked in pie-radio), it makes for quite a nice experience, in my opinion. In this case it makes it easier to reopen the toast once it has dismissed itself.
  2. I noticed that if you load the toast story with isDismissible = false, the close button is still shown: https://pr1862-storybook.pie.design/?path=/story/toast--auto-dismiss&args=isDismissible:!false
  3. If you set the duration to 10000 and refresh the page to restart the timer, then after 5 seconds close and immediately reopen the toast, it will close again once the original 10 seconds is finished. For me the expected behaviour is that the timer should be reset when the toast is reopened.
  1. has been solved. 2 and 3 needs more time to investigate which we can involve more people to fix these issues.

@thejfreitas
Copy link
Member Author

/test-aperture

@pie-design-system-bot
Copy link
Contributor

Starting a new snapshot build. You can view the logs here.

@pie-design-system-bot
Copy link
Contributor

The build failed, please see the logs or take a look at the Workflow Tooling wiki page to make sure your PR meets the requirements.

xander-marjoram
xander-marjoram previously approved these changes Oct 2, 2024
Copy link
Contributor

@xander-marjoram xander-marjoram left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this because it all seems to work well and I don't want to stand in your way too much. I would appreciate if you could address my request to add that comment, but that can be as part of a follow-up PR if you like.

jamieomaguire
jamieomaguire previously approved these changes Oct 2, 2024
jamieomaguire
jamieomaguire previously approved these changes Oct 2, 2024
@thejfreitas
Copy link
Member Author

I'm happy to approve this because it all seems to work well and I don't want to stand in your way too much. I would appreciate if you could address my request to add that comment, but that can be as part of a follow-up PR if you like.

Since my approvals were reset I took the opportunity to add the comments in this PR already. I totally missed that comment.
Its done ✅

@jamieomaguire jamieomaguire merged commit c5ba7c9 into main Oct 3, 2024
34 checks passed
@jamieomaguire jamieomaguire deleted the dsw-2204-pie-toast-auto-dismiss-persistent-feat branch October 3, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants