fix(toast): deduplicate toasts with same key

[foodforarabbit]

Fixes #2333

Description

• fixed typing on some of the functions. There was a type ToastPropsShapeT (ToastPropsT minus children) being used for state.toasts and for the return value of getToastProps(), but actually the children prop is in both. children is separate in the toaster.show() function but then it is recombined in the toasterInstance.show() function.
• added helper function updateToastsMutationWithCount which performs the original toast update mutation but also returns the number of toasts updated
• added updateToastsMutationWithCount to toasterInstance.show() function - it will attempt to update toasts with matching key prop first, and then only push the new toast if nothing was updated
• clear existing autohide timeout in Toast.startTimeout if it already exists (i.e. reset the autohide timer)
• call Toast.startTimeout in componentDidUpdate ONLY if autoHideDuration property is changed - we can avoid a deep comparison to figure out if a toast has changed or not. Might want to make this more explicit somehow - suggestions welcome
• used Layer instead of creating a portal manually to the body
• added new scenario and e2e test for normal toast behaviour (no key provided, autogenerate key and pop up additional toasts) and same-key behaviour (if toast with same key found, update it instead of popping up a new one)
• added some text on the documentation page about toaster utility and the same-key update situation

Scope

• Patch: Bug Fix
• Minor: New Feature
• Major: Breaking Change

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/uber-ui-platform/baseweb/8hbcmffgn
✅ Preview: https://baseweb-git-fork-foodforarabbit-toast-de-duplicate.uber-ui-platform.now.sh

[foodforarabbit]

actually I was thinking we could implement a hidden counter in the toast config (e.g. __updated) that is increased every time a toast is updated. That way, we could know when to reset the autohide timer without any expensive comparisons while also making it automatic (right now I'm simply comparing autoHideDuration so to reset the autohide timer, you need to change this. Seems a bit hacky though). Not sure if this discussion belongs here or in the original issue though.

something like this:

    toasts.forEach((t, index, arr) => {
      if (t.key === key) {
        const toastUpdateCount = parseInt(t.__updated) || 0
        arr[index] = {...t, __updated: toastUpdateCount + 1, ...this.getToastProps(props), key};
      }
    });

[UberOpenSourceBot]

Visual changes were detected on this branch. Please review the following PR containing updated snapshots: foodforarabbit#1

[houmark]

Excited about this PR. I did not have the chance to test it out yet, but it seems like everything annoying about toasts fixed :) Nice work.

[houmark]

About the autoHideDuration — Do I understand it correctly, that to extend an updated toast that had 10000 in its first show, to have another 10 seconds, I will need to submit the update/second show with 10001 for example? Basically a different value than the first time? If I do not submit a value or the same, it will hide 10 seconds after showing the first toast?

A different approach to both of those could be a new prop extendAutoHideDuration so I can decide when updating if I want it to extend the time or not (seems less hacky), but I don't have a strong opinion on either one of the solutions.

[chasestarr]

Looks like it's heading in the right direction 👍

I agree that changing the autoHideDuration is not sufficient, and the update count approach seems more appropriate. Another prop called resetAutoHideTimerOnReset or similar can be added to control the behavior. If 'showing' a duplicate toast should it reset the timer by default, or the opposite? I personally am leaning towards reset.

[houmark]

I agree that changing the autoHideDuration is not sufficient, and the update count approach seems more appropriate. Another prop called resetAutoHideTimerOnReset or similar can be added to control the behavior. If 'showing' a duplicate toast should it reset the timer by default, or the opposite? I personally am leaning towards reset.

Just a naming nitpick. resetAutoHideTimerOnReset should maybe be resetAutoHideTimerOnUpdate?

[foodforarabbit]

I agree that changing the autoHideDuration is not sufficient, and the update count approach seems more appropriate. Another prop called resetAutoHideTimerOnReset or similar can be added to control the behavior. If 'showing' a duplicate toast should it reset the timer by default, or the opposite? I personally am leaning towards reset.

Just a naming nitpick. resetAutoHideTimerOnReset should maybe be resetAutoHideTimerOnUpdate?

caught this already :D

[chasestarr]

I agree that changing the autoHideDuration is not sufficient, and the update count approach seems more appropriate. Another prop called resetAutoHideTimerOnReset or similar can be added to control the behavior. If 'showing' a duplicate toast should it reset the timer by default, or the opposite? I personally am leaning towards reset.

Just a naming nitpick. resetAutoHideTimerOnReset should maybe be resetAutoHideTimerOnUpdate?

caught this already :D

🙄 ah good catch, typo

[foodforarabbit]

hehe well I'll leave it up to them to decide. I'm still new around here, just fixing bugs and filling in gaps. Don't know what the long term plans are yet - was going to finally get started looking over the figma tomorrow! but I believe that if they are going to allow it at all, props on a toast is the lesser evil compared to two ToasterContainers controlled by directly by ref puppet strings xD

…

On Tue, Jan 21, 2020 at 11:09 PM houmark ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/toast/toaster.js <#2657 (comment)>: > @@ -41,6 +41,15 @@ export class ToasterContainer extends React.Component< constructor(props: ToasterPropsT) { super(props); + + if (__DEV__) { + if (toasterRef !== undefined) { + // eslint-disable-next-line no-console + console.warn( + 'A ToasterContainer already exists in your application. It is not recommended to have more than one ToasterContainer in an application.', Also just thought if placement could be controlled by the toast itself (the ToasterContainer prop can jsut be default like autoHideTimeout is) that would cover your use case with a single ToasterContainer as well. It shouldn't be too hard to sort toasts based on placement and render them separately. But again, another time, another issue :) I'd personally love to see that (in a different PR of course). It would make it simpler and then this warning could be reintroduced and the old behavior deprecated in the warning with a link to the documentation for that new feature. But as you may read in the issue I created to ask for this, it did not seem like this was a feature the Uber team wanted as it was not good practice, but they may be more open to it now (as they had a case where they helped someone implement it after I did my question)? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2657?email_source=notifications&email_token=AAR5OZZF24O4TNW62ZLXDIDQ67WKNA5CNFSM4KIXQPV2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSSVM2A#discussion_r369398236>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5OZ2OXSH6YDIA74B5HYDQ67WKNANCNFSM4KIXQPVQ> .

[UberOpenSourceBot]

Visual changes were detected on this branch. Please review the following PR containing updated snapshots: foodforarabbit#1

[chasestarr]

I think this is good to land once VRT changes are resolved. Thanks for looking into the auto-hide issue and raising the some of the complexities with multiple containers. I'm open to start addressing the placement-by-toast feature. It's not a common pattern, but there's at least one application here that needs it

[foodforarabbit]

I think this is good to land once VRT changes are resolved.

Not sure what I need to do with the VRT xD I saw a pull request into my branch and merged it in, saw 3 pngs but not sure what I was looking for.

[chasestarr]

It's kind of a side-effect of how we combine e2e tests and visual regression tests. In this case, the new screenshots look fine