localstorage for alert #11
Conversation
There was a problem hiding this comment.
Overall, good job, it seem to work fine.
Just one other thing to think about, that localstorage behavior should be a opt-in behavior. So one thing you could do is have a prop for the component, like stopShowingWhenClosed, and the value of the prop could be the number of time you want the alert to be closed (in that case the hardcoded 5 you used). If that prop is not set (undefined) then the behavior shouldn't be used and the alert should always be shown no matter what.
| @@ -57,34 +60,39 @@ export default class Alert extends React.Component { | |||
| } | |||
|
|
|||
| render() { | |||
| const { children, type, unclosable } = this.props | |||
| if (this.state.count < 5) { | |||
| console.log(this.state.count) | |||
There was a problem hiding this comment.
debug log : can be removed
| @@ -1,6 +1,7 @@ | |||
| import React from 'react' | |||
| import PropTypes from 'prop-types' | |||
| import styles from '../styles.less' | |||
| import { loadstate, saveState } from './localStorage' | |||
There was a problem hiding this comment.
just for the sake of having the same naming convention, you could rename loadstate to loadState (capital S)
| const { children, type, unclosable } = this.props | ||
| if (this.state.count < 5) { | ||
| console.log(this.state.count) | ||
| saveState(this.state.count + 1) |
There was a problem hiding this comment.
So I think the behavior should rather be "if the user closes the popup 5 times we should stop showing it" rather than having the render function incrementing the number. To do so, you can use the close() method.
| @@ -0,0 +1,20 @@ | |||
| export const loadstate = () => { | |||
| try { | |||
| const serializedState = localStorage.getItem('count') | |||
There was a problem hiding this comment.
One problem I see with that implementation is that if you have several alert components, they will all share the same localStorage variable count, and will interfere with each other. To fix that, the localstorage key name should maybe be passed as a prop to the Alert component.
There was a problem hiding this comment.
Good job!
You can also have a look into adding some documentation in the component's README file to document your changes and your new props. Additionnaly, you can add a Storybook story for that component with the new props that you added.
Also, you can add some tests for this behavior
|
|
||
| if (this.state.count < this.props.stopShowingWhenClosed || this.props.stopShowingWhenClosed === undefined) { |
There was a problem hiding this comment.
You can unwrap stopShowingWhenClosed info in the const {...} = this.props line above
| onClose: PropTypes.func, | ||
| } | ||
|
|
||
| static defaultProps = { | ||
| children: '', | ||
| type: 'info', | ||
| unclosable: false, | ||
| stopShowingWhenClosed: undefined, | ||
| componentKey: '', |
There was a problem hiding this comment.
Since this prop is really tied with stopShowingWhenClosed, maybe you should name it accordingly (for instance stopShowingWhenClosedKey (which is super long, I agree, but maybe it's better to understand what it really means)
…o fyndiq-ui-new-alert
Create a local storage for "Merchant Warning Alert for Revenue" after 5 attempts it will be disappear.