Fix #1884. Add cookie policy notification #1890
Conversation
offtherailz
commented
May 29, 2017
offtherailz
commented
- Add Notification system
- Add cookie policy epic to show the cookie policy notification
- Add Notification system - Add cookie policy epic to show the cookie policy notification
| this.updateNotifications(this.props.notifications); | ||
| }, | ||
| shouldComponentUpdate(nextProps) { | ||
| return this.props !== nextProps; |
There was a problem hiding this comment.
Not needed, this should be redux connect default
There was a problem hiding this comment.
It protects the component for a future reuse in other contexts.
There was a problem hiding this comment.
| @@ -130,6 +130,7 @@ | |||
| "react-input-autosize": "1.1.0", | |||
| "react-intl": "2.2.3", | |||
| "react-joyride": "1.10.1", | |||
| "react-notification-system": "0.2.14", | |||
There was a problem hiding this comment.
Uhm... it seems to me this module is not reactish at all if we use the NotificationSystem component. Basically it's the most imperative component I have seen til now. It would be better to use the NotificationsContainer directly, in my opinion, or use https://github.com/gor181/react-notification-system-redux that is a wrapper hiding the imperative stuff
There was a problem hiding this comment.
Yes, i don't like very much the implementation too. This is why I wrote the wrapper, that allows to use it in a one way data flow (as many of our existing components).
Maybe in the future we can replace it modifying only the wrapper (the model for the notification is very simple, so it can be replaced quickly), or maybe they will improve the lib.
But for the moment we have a notification system that works, without creating divs here and there to notify success, warning or errors.
I've seen the wrapper you linked. It doesn't work as well as expected in our use case:
- Uses the store in the context
- doesn't seems to work on mount, but only on update (probably the author didn't understand the difference between
componentDidUpdateandcomponentWillReceiveProps).
For our plugin system this solution is better in terms of maintenance.
| @@ -130,6 +130,11 @@ | |||
| } | |||
| } | |||
| }, | |||
| "cookiesPolicyNotification": { | |||
| "title": "Questo sito utilizza cookies", | |||
| "message": "Continuando a navigare sul sito accetti all'uso dei cookie", | |||
web/client/reducers/notifications.js
Outdated
| const { type, ...rest } = action; | ||
| return [ | ||
| ...state, | ||
| { ...rest, uid: action.uid} |
There was a problem hiding this comment.
Shouldn't uid already be taken from rest?
| this.updateNotifications(this.props.notifications); | ||
| }, | ||
| shouldComponentUpdate(nextProps) { | ||
| return this.props !== nextProps; |
There was a problem hiding this comment.
