add hideToast prop to authenticator

[Luke-Davies]

Issue #, if available:
None

Description of changes:
Adds a property to the amplify-authenticator called showToastshideToast which is defaulted to false.

Setting this to true will prevent amplify-toast from rendering when TOAST_AUTH_ERROR_EVENT events are raised on the UI_AUTH_CHANNEL.

This is useful if you want to roll your own alerting components for auth error messages.

React Example:

import React, { useState, useEffect } from 'react';
import { Hub, HubCallback } from '@aws-amplify/core';
import {
  AmplifyAuthenticator,
} from '@aws-amplify/ui-react';
import {
 UI_AUTH_CHANNEL, TOAST_AUTH_ERROR_EVENT
} from '@aws-amplify/ui-components';

const MyAuth: React.FC = ({ children }) => {

  const [alertMessage, setAlertMessage] = useState('');

  const handleToastErrors: HubCallback = ({ payload }) => {
    if (payload.event === TOAST_AUTH_ERROR_EVENT && payload.message) {
      setAlertMessage(payload.message);
    }
  };

  useEffect(() => {
    Hub.listen(UI_AUTH_CHANNEL, handleToastErrors);
    return () => Hub.remove(UI_AUTH_CHANNEL, handleToastErrors);
  });

  return (
    <>
      {alertMessage && (<div>{alertMessage}</div>)}
      <AmplifyAuthenticator hideToast>
        // ...........
      </AmplifyAuthenticator>
    </>
  );
}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[codecov]

Codecov Report

Merging #7129 (7d33156) into main (6c9c4ef) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #7129   +/-   ##
=======================================
  Coverage   74.26%   74.26%           
=======================================
  Files         215      215           
  Lines       13470    13470           
  Branches     2645     2645           
=======================================
  Hits        10004    10004           
  Misses       3268     3268           
  Partials      198      198           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c9c4ef...7d33156. Read the comment docs.

[ThrowingSpoon]

Could this get reviewed please? Would be awesome to have 💪

[Luke-Davies]

Not sure if I'm supposed to randomly bug people for reviews sorry! @elorzafe @sammartinez @manueliglesias

This is a pretty simple change.

Could argue that it should be hideToasts instead of showToasts to match this prop in amplify-sign-in. Opinions welcome.

[wlee221]

Sorry about the late review! Just got some nits but looks good to me otherwise 👍

[Luke-Davies]

Thanks @wlee221!
Both changes made and tested locally.
I've updated the PR name & description to reflect the prop change from showToasts --> hideToast

[ericclemmons]

Thanks for taking this need on @Luke-Davies.

I support @wlee221's feedback, and wanted to offer some context into past discussions around this need.

The UI components are, as we know, powered by Web Components and often rely on "slots" for customizing individual components.

What we didn't know at the time is if hideToast would be equivalent to overriding an amplify-toast slot (if it existed):

<AmplifyAuthenticator>
  <!-- Default components here -->
  <div slot="amplify-toast">
    <!-- Nothing to see here! -->
  </div>
</AmplifyAuthenticator>

Do either of y'all know if this is better, worse, or possible? IIRC, the rationale for hideSignUp was because the sign-up language was "too deep" in the component to replace with an empty slot:
https://github.com/aws-amplify/amplify-js/pull/6098/files

Again, I'm supportive of this PR to unblock customers! I just wanted to share where we left off in conversations in the past.

[Luke-Davies]

Thanks for the context @ericclemmons. I had considered trying to implement this feature by introducing a slot and would absolutely support that solution if it is more in-keeping with the design choices made for the project so far.

If a slot is introduced then I would suggest something other than amplify-toast for the name.

For example; our app displays a modal for auth errors because we found that some customers were not seeing the toast on certain devices unless they happened to scroll to the top of the page. Currently, we're showing this modal as well as the toast (until we can hide the toast).

Perhaps amplify-auth-alert or just amplify-alert would be a better name for the slot if that approach is preferred?

[ericclemmons]

I agree, amplify-toast isn't great. It matches the original desire of a "toast" notification, but we also considered amplify-notification among others.

I just found #6479 as well, so we're not the only ones who've talked about this :D

So, in terms of preference having fewer props would be preferential. Especially since a slot would allow customization compared to hiding entirely.

@wlee221 What are your thoughts on the matter? Do you prefer props vs. slots, and what name would you use? (Assuming it would work!)

[wlee221]

I can't believe I missed the slot approach! I just tested that using slots works as intended, so this is really up to what we want to do.

I'm kind of divided on slots vs props though. If it were just for hiding toast, I would have argued that hideToast is more intuitive for customers. I find it unintuitive to pass an empty element to slot to hide a section:

<amplify-authenticator>
  <div slot="amplify-toast"></div>
</amplify-authenticator>

And it's not clear what the code is trying to do on first glance. I think the purpose of hiding an element deviates from customization, which slot specializes in.

That being said, customers are asking for toast customizations on #6479. Slots do well for covering both use cases, so I think slot is the best way to go. And I agree with @ericclemmons' comment that fewer props are preferential.

So tldr; we should go with slots. For the slot name, I like amplify-alert or just alert because it conveys that there's an error better than other suggestions imo.

[Luke-Davies]

@wlee221 @ericclemmons
I've raised a new PR for the slotted approach. Please see PR #7601

I believe we can close this one.

[Luke-Davies]

Coming back to this following a discussion on #7601 (slot approach). To understand why we've come back to props, see this thread.

[Luke-Davies]

@wlee221 One final change I wasn't sure of on this was whether to wrap Hub.listen(UI_AUTH_CHANNEL, this.handleToastEvent); and Hub.remove(UI_AUTH_CHANNEL, this.handleToastEvent); with if (!this.hideToast). The prop could technically be updated though (I'm not sure why anyone would) which could result in the callback not being removed.

Let me know if you think it's worth doing.

[wlee221]

I think this is a pretty simple optimization worth doing. As mentioned, I don't see a good reason for changing the hideToast value dynamically.

[wlee221]

This LGTM 👍 Thanks a lot for your contribution throughout this!

[github-actions]

This pull request has been automatically locked since there hasn't been any recent activity after it was closed. Please open a new issue for related bugs.

Looking for a help forum? We recommend joining the Amplify Community Discord server *-help channels or Discussions for those types of questions.