iamhosseindhv / notistack

Highly customizable notification snackbars (toasts) that can be stacked on top of each other
https://notistack.com
Other
3.93k stars 298 forks source link

[Help needed] [Redux] Closing snackbar programmatically #116

Closed favna closed 5 years ago

favna commented 5 years ago

Hello,

I've mostly been going by your example on how to use the lib in redux setting while also implementing it in such a way that we can trigger snackbars from our redux-saga. However today I was working on also implementing that the snackbar closes when the "dismiss" button is clicked however for the life of me I cannot pull this off. I would appreciate some help on this.

Here is a Gist with copies of the relevant files as we use them:

https://gist.github.com/Favna/7725ea47c8835d5587625f2eb80bddee

(Note: It's in TypeScript and with Create-React-App!)

p.s. I see on an older closed issue that you removed a default action behaviour from SnackbarProvider. Why is this? This feature would be amazing to have when you have the same action on every snackbar in the app, which would be the case for us. In fact, ideally something like this would be best:

<SnackbarProvider closeAction={true}>
    <App />
</SnackbarProvider>
favna commented 5 years ago

@iamhosseindhv could you check out this issue? I see 2 other peers have upvoted it, presuming they are facing the same problem.

iamhosseindhv commented 5 years ago

@Favna I have attempted to come up with a solution for this, even before you opened the issue. However, since it was starting to get a long time, I've postponed working on this to after my university exams are finished (in 2 weeks).

Thanks for your patience. ❤️

favna commented 5 years ago

That's quite alright. School's very important ;)! Best of luck with that and I'm looking forward to the solution.

tylertyssedal commented 5 years ago

I also cannot get this to work with Redux.

From the example provided in the docs, you are calling the removeSnackbar() action on every update within the Notifier component, which clears out the state's list of notifications. It seems as if closeSnackbar() is not actually being called in the Notifier? Because, ideally, you'd want to tie the notifications to the state, so that you would be managing which notifications are still persisting based on that.

I would like to create a notification with persist: true and dismiss it later, ideally through updating the Redux store. Right now, there seems to be no way to actually dismiss any notifications in the Redux example.

So, to clarify, the actual workflow that I'd be looking to accomplish looks like this, utilizing redux-thunk to chain dispatches:

  1. const retrievingDispatch = dispatch(enqueueSnackbar({ message: 'Retrieving...', options: { persist: true }})), grabbing the key from the retrievingDispatch object
  2. Wait until a fetch Promise completes, then dispatch(removeSnackbar(key))

The current Redux example doesn't work with this, as the Notifier component doesn't seem to care. Additionally, setting Notifier's componentDidUpdate() to simply listen to an update and dispatch closeSnackbar() on that key also doesn't dismiss the persisted notification.

Please advise. Would love to use your library, but I need programmatic access to managing the notifications based on Redux state in order to do so properly.

Thanks.

favna commented 5 years ago

@metamet Good that you brought up Redux-Thunk. I've been so delved into Saga that I completely forgot about it. I've updated the title of the issue to be more generic so it covers both libraries (as well as self-written middleware but who would ever do themselves that displeasure)

tylertyssedal commented 5 years ago

Thank you @Favna . Glad I could be a helpful reminder of it. :)

In the meantime, do you suspect the library is currently capable of the use case we're looking to achieve here, and it's just a matter of wiring it up properly? Or is there further development that first needs to occur?

favna commented 5 years ago

@metamet Honestly I'm not sure. I have a feeling that it has to be possible some way and depends on the implementation but I have no view on the actual how for that. It's about 2 weeks since Hossein said he was finishing his exams "in 2 weeks" so I can only assume he'll soon have time to look into things.

tylertyssedal commented 5 years ago

So I may have figured out the source of the issue when it comes to dismissing the notification...

The Redux example shows an action that creates a new notification. That notification, however, is creating a new key:

      notification: {
          key: new Date().getTime() + Math.random(),
          ...notification,
      },

This key is only used in reference to the notifications array, and not a valid key to be used in dismissing notifications. In the Notification component, however, we're just running this.props.enqueueSnackbar(message, options). If you return the result of that, it's not the same key.

So what we actually need here is a way to override the key or manually set the key when running enqueueSnackbar(), since the state is depending upon the key we set manually.

I've rewritten the Redux example and will happily do a PR later on. But I think that's the source of the issue with the Redux example. The key isn't the notification key--just an arbitrary reference inside of the state, so we can't rely on it to dismiss.

favna commented 5 years ago

In the codebase where I'm working the key is often enough a UUID-v4 of a user that is, for example, being logged in so it's actually quite reliable to reference however using that I could still not programatically dismiss the notification. Maybe I was doing something wrong because that was among the many other solutions I tried but I would cheer too soon and first confirm it yourself.

tylertyssedal commented 5 years ago

Are you able to dismiss all notifications by not passing a key to closeSnackbar()?

favna commented 5 years ago

@metamet yes by not passing any key to closeSnackbar() all snackbars would get dismissed. However unless I'm overseeing something I do not see how to supply the action to the thunk/saga enqueuement of the action, and when adding it to the global SnackbarProvider the app crashes by not being able to read handleEnqueueSnackbarProvider of undefined. You can see this behavior in the sample code as talked about below

I have created a very basic app to show both a fully working snackbar enqueue based on button click with dismiss action (based on the example entirely) and also some Redux-Saga where I enqueue snackbars when a "increase counter" and "decrease counter" buttons are pressed. You can find it here: https://github.com/Favna/notistack-saga-sample

I'd say lets use this sample app as a basis for getting it to work. Once we get it to work in Saga I'm sure we can apply the same logic to Thunk.

I've added some code in both App.tsx and homeSaga.ts where I see options to add the closing of snackbars, but they both fail. Maybe I'm just greatly overlooking something here so if you see the solution do share (or fork the repo and PR it there I suppose)

tylertyssedal commented 5 years ago

So here's where I believe the problem exists:

export const enqueueSnackbar = (snackbar: Snackbar) => action(SnackbarActionTypes.ENQUEUE_SNACKBAR, {
    key: (new Date().getTime() + Math.random()).toString(), ...snackbar,
});

https://github.com/Favna/notistack-saga-sample/blob/master/src/store/snackbars/snackbarActions.ts

This key being generated here is only used in the store. It's not actually referencing the notistack notification key.

The actual key required to dismiss the notification would be found here:

            this.props.enqueueSnackbar(snackbar.message, snackbar.options);

https://github.com/Favna/notistack-saga-sample/blob/master/src/containers/Notifiers.tsx#L40

If you change that line to...

  let actualKey = this.props.enqueueSnackbar(snackbar.message, snackbar.options);
  console.log(actualKey)

I believe you'll retrieve the key actually required to remove the snackbar. For example, in that code, if you change the block to this...

    componentDidUpdate () {
        this.props.snackbars.forEach((snackbar: Snackbar) => {
            if (!snackbar.key) return;

            let actualKey = this.props.enqueueSnackbar(snackbar.message, snackbar.options);
            this.storeDisplayed(snackbar.key);
            this.props.removeSnackbar(actualKey);
            this.displayed.filter(s => s !== snackbar.key);
        });

The snackbar either wont display, or will show up for a split second before it's removed.

I think the store's key generation has been a red herring, since it actually doesn't do anything outside of reference the store's object and adds confusion.

So what we need is a way to either manually set the key that's returned from enqueueSnackbar, or to manually update the store upon enqueue to reference the actual key notistack uses.

Does that make sense?

tylertyssedal commented 5 years ago

@Favna I've got it working with Redux: https://github.com/metamet/notistack/tree/master/examples/redux-example

I had to rebuild the Redux example from the ground up, but it's working now. There are a few quirks (native to notistack, as I note in the comments throughout...), but the Notifier I created and the way it relates to the Redux state and actions allows you to programmatically dismiss notifications.

One downside is that, due to the way notistack is currently designed, the best way to ensure that all notifications can be dismissed from the stock is to store the notification stack in state. I have a removeSnackbar action to remove them from store, but notistack would need to manually trigger an update upon dismissal in order to trigger it without creating odd race conditions.

Let me know if this helps you.

@iamhosseindhv I would be happy to merge my Redux example into your repo whenever you're ready.

favna commented 5 years ago

It's a great effort but sadly it would not work for me @metamet. Keeping our Redux stuff (actions, sagas, reducers) in .ts as opposed to .tsx is important to our style of code so it's not possible to supply a custom JSX element as children in the enqueueing. I've also tried to adopt your code in my own sample, but I couldn't pull it off on the basis of it conflicting too much with how typesafe-actions and typescript standards in general manage the state (at least for what I can tell).

I'll keep to it that it would be very much appreciated still if this would be possible as it would mean very minimal effort on the end-user side, instead of a whole workaround like you described.

import IconButton from '@material-ui/core/IconButton';
import { createMuiTheme } from '@material-ui/core/styles';
import CloseIcon from '@material-ui/icons/Close';
import { StylesProvider, ThemeProvider } from '@material-ui/styles';
import history from 'config/history';
import Router from 'config/Router';
import configureStore from 'config/store';
import { ConnectedRouter } from 'connected-react-router';
import { SnackbarProvider, useSnackbar } from 'notistack';
import React, { FC } from 'react';
import { Provider } from 'react-redux';

const App: FC = () => {
  const initialState = {} as any;
  const store = configureStore(initialState);

  const theme = createMuiTheme({
    palette: {
      type: 'dark', // Switching the dark mode on is a single property value change.
    },
  });

  /**
   * This will have the app crash on not being able to read "handleEnqueueSnackbar of undefined".
   * Not sure why that happens actually. I feel like it can be worked around.
   * Adding the action here would be most ideal since it'll add the close action to every snackbar
   * throughout the entire app.
   */
  const { closeSnackbar } = useSnackbar();

  const action = (key?: ReturnType<typeof closeSnackbar>) => (
      <IconButton onClick={() => key ? closeSnackbar(key) : closeSnackbar()}>
          <CloseIcon />
      </IconButton>
  );

  return (
    <Provider store={store}>
      <ConnectedRouter history={history}>
        <StylesProvider injectFirst>
          <ThemeProvider theme={theme}>
          <SnackbarProvider maxSnack={4} action={action}>
            <Router />
          </SnackbarProvider>
          </ThemeProvider>
        </StylesProvider>
      </ConnectedRouter>
    </Provider>
  );
};

export default App;
tylertyssedal commented 5 years ago

Understood.

So I think the most direct fix to this would be to have an additional, optional parameter for notistack's enqueueSnackbar() to allow for manually setting the notification key, so as you could track its dismissal key outside of awaiting the return. This could allow whatever state management solution to manage and track the keys anywhere in the notification pipeline.

favna commented 5 years ago

I went on a bit of a search for other notifications libs including those not using material-ui and I found react-redux-toastr which is based on the popular JQuery lib toastr and as the name implies inherently also uses Redux. I've just set it up on my previously linked repo in a branch. I'll still have to do some thinking about tracking IDs when triggering them from saga but I'm sure I can figure it out - it's mostly just what I exactly want to store.

All in all since these notifications are to be used in a production app at my job and our UX'ers have also asked me to see into implementing them into a lib that uses React 15 (and thus no Material UI) I think we'll start looking into this other lib. Would be really nice to see notistack grow in it's usage still though!

iamhosseindhv commented 5 years ago

Sorry for being late in the discussion, I was working on the new documentation website: https://iamhosseindhv.com/notistack

As already identified by you guys, the problem was originated in the fact that the key in the redux store was different from the one generated by notistack.

@metamet So what we need is a way to either manually set the key that's returned from enqueueSnackbar, or to manually update the store upon enqueue to reference the actual key notistack uses.

We already can pass our own key in the options parameter of enqueueSnackbar: https://github.com/iamhosseindhv/notistack/blob/master/src/SnackbarProvider.js#L62

I've updated codesandbox example to reflect fixes.

Final point: if you're using redux and want to be able to make use of closeSnackbar, you should pass your own key in the options parameter of enqueueSnackbar.

Thanks all.

Paul-Vandell commented 5 years ago

Hi there, Yeah its closed, but for anyone who have lost 2 hours like me ...

 componentDidUpdate() {
        const { notifications = [] } = this.props;

        notifications.forEach(({ key, message, options = {} }) => {
            if (this.displayed.includes(key)) return;
            this.props.enqueueSnackbar(message, {
                ...options,
                key,  <========= | JUST ERASE THIS KEY HERE |
                onClose: (event, reason, key) => {
                    if (options.onClose) {
                        options.onClose(event, reason, key);
                    }
                    // Dispatch action to remove snackbar from redux store
                    this.props.removeSnackbar(key);
                }
            });
            this.storeDisplayed(key);
        });
    }

With this i can retrieve my correct key on the action property :

<SnackbarProvider
        maxSnack={3}
        preventDuplicate={true}
        anchorOrigin={{
          vertical: "bottom",
          horizontal: "left"
        }}
        action={key => (
          <IconButton
            onClick={() => dispatch(YOUR_ACTION(key))}
          >
            <CloseRounded className={classes.closeIcon} />
          </IconButton>
        )}
      >

It's working for me. Anyway. A big thx for this library

iamhosseindhv commented 5 years ago

@Vandell63 I guess the problem we originally had was to close a snackbar programmatically, as in without user interaction. Your solution works perfectly fine when someone clicks the close button, but not when you want to close a snackbar yourself.

Paul-Vandell commented 5 years ago

Oh yes i see , so you have to pass your key in notistack options when you enqueue it to get it programmatically later ?. All fine thx to you

iamhosseindhv commented 5 years ago

@Vandell63 Exactly, you should pass your own key if you're using redux and planning to dismiss it programmatically later.

tylertyssedal commented 5 years ago

@iamhosseindhv You said...

Final point: if you're using redux and want to be able to make use of closeSnackbar, you should pass your own key in the options parameter of enqueueSnackbar.

I am still not getting this to work. I have confirmed that the options being passed in to enqueueSnackbar are {variant: "info", key: 1562095241521.1873}. Calling this.props.closeSnackbar(1562095241521.1873) within a withSnackbar() component does not dismiss it.

Am I missing something here?

Edit: Yes. I think I figured it out here. options.key must be a string.

Is this intentional?

brownieboy commented 5 years ago

Here's how I reworked the Redux example code to work with Sagas:

https://github.com/brownieboy/notistack-redux-saga-example

Roy447 commented 3 years ago

How to test a global store redux in unit testing?