fix(OverlayToaster): Don't dismiss excess toasts when updating a toast while maxToasts is set

[jscheiny]

Checklist

• Includes tests
• Update documentation (N/A)

Changes proposed in this pull request:

Something of a niche case, but noticed it while iterating on a different toaster issue and wanted to quickly fix it. This issue occurs if there are a number of toasts on the screen equal to the maxToasts prop. Previously, if you updated one of those toasts, a toast would be dismissed unnecessarily (which could be bad if its meant to be an indefinitely present toast). Now, if a toast is updated, we don't do the max toasts check since the number of toasts hasn't changed.

This PR also contains some changes that will help with further refactors to the overlay toaster component.

[changelog-app]

Generate changelog in packages/core/changelog/@unreleased

What do the change types mean?

• feature: A new feature of the service.
• improvement: An incremental improvement in the functionality or operation of the service.
• fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
• break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
  and external consumers of the service's API (e.g. customer-written software or integrations).
• deprecation: Advertises the intention to remove service functionality without any change to the
  operation of the service itself.
• manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
  performing database surgery, ...) at the time of upgrade for it to succeed.
• migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

• ❗The break and manual task changelog types will result in a major release!
• 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
• ✨ All others will result in a minor version release.

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

fix(OverlayToaster): Don't dismiss excess toasts when updating a toast while maxToasts is set

Check the box to generate changelog(s)

• Generate changelog entry

[svc-palantir-github]

fix(OverlayToaster): Don't dismiss excess toasts when updating a toast while maxToasts prop is set

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

[svc-palantir-github]

Add generated changelog entries

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

[ggdouglas]

I've noticed that the current abstraction with this function taking another function as an argument is making the code a bit harder for me to follow. It seems a bit premature to abstract it in this way.

Would it be possible to simplify by pulling the check for updating an existing toast to the top and using it directly in the conditional for dismissing? e.g.

public show(props: ToastProps, key?: string) {
    const options = this.createToastOptions(props, key);
    const isUpdatingExistingToast = key != null && !this.isNewToastKey(key);

    if (this.props.maxToasts && !isUpdatingExistingToast) {
        // check if active number of toasts are at the maxToasts limit
        this.dismissIfAtLimit();
    }

    this.setState(prevState => {
        const toasts = isUpdatingExistingToast
            ? // update a specific toast
              prevState.toasts.map(t => (t.key === key ? options : t))
            : // prepend a new toast
              [options, ...prevState.toasts];

        return { toasts, toastRefs: this.getToastRefs(toasts) };
    });
    return options.key;
}

[jscheiny]

I agree that its a bit premature in the context of this PR, but with my FLUP change for queuing, it becomes more needed as we're doing these state updates in a couple of places where its not so easy to refactor. If you're down, lets leave this as is and maybe in the context of the other PR we can reevaluate?

[svc-palantir-github]

CR

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.