-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create a snackbar to notify the user of important events #226
base: master
Are you sure you want to change the base?
Conversation
…on closing button
@@ -0,0 +1,23 @@ | |||
import styled from 'styled-components'; | |||
import { theme } from '../../styling/theme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't import the theme from file, this doesn't allow it to update if the theme changes.
import { theme } from '../../styling/theme'; | ||
|
||
export default styled.button` | ||
background-color: ${theme.palette.primary.main}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be
background-color: ${theme.palette.primary.main}; | |
background-color: ${props => props.theme.palette.primary.main}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See example here
position: absolute; | ||
right: 10px; | ||
top: 1.2em; | ||
color: ${theme.palette.primary.contrast}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
margin-left: 1em; | ||
|
||
&:hover { | ||
color: ${theme.palette.danger.main}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
import React from 'react'; | ||
import styled from 'styled-components'; | ||
import SnackBarCloseButton from './SnackBarCloseButton'; | ||
import { theme } from '../../styling/theme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well. Won't comment anymore on this as I think you get the gist ;)
interval_type, | ||
start_date: date, | ||
template: { | ||
if (!(description.length > 140) && !(notes.length > 140)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it's probably better to use guard statements. So instead of wrapping everything in an if
-statement, just check your stuff, do your "YOU DID NOT ENTER THINGS ACCORDING TO HOW I WANT THEM"-snack-dispatching, and then return;
. That way, you don't have to indent everything and increase the "legibility"-complexity.
if (!validLength) {
toast('invalid length');
return;
}
if (!validFormat) {
toast('invalid format');
return;
}
// etc.
|
||
interface ISnackBarProps { | ||
className?: string; | ||
clicker?: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name should be updated, this doesn't make it very clear what it does. If it's a handler, use the appropriate type.
} | ||
|
||
interface IAction { | ||
type: 'clear' | 'good' | 'bad'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh, already wrote a lot of comments, but updating the file removed them all 🙃
This doesn't have to be an object.
type SnAction = 'clear' | 'good' | 'bad';
import Form from './Form'; | ||
|
||
const initialState = { snax: <div />, content: '' }; | ||
|
||
interface IState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface IState { | |
interface ISnackStore { |
type: 'clear' | 'good' | 'bad'; | ||
} | ||
|
||
const reducer = (state: IState, action: IAction): IState => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so there are quite a few things here that need to be fixed. Already wrote some of them that later disappeared, so I'll be a bit less verbose this time.
- Reducer should be moved to its own file.
- You generally shouldn't store render elements in the store. This is bad practise, and will generally result in a lot of trouble. You have no way to update this element, and it will have to be recreated every time the reducer is updated (I think). Furthermore, how it is mounted in the DOM is not clear, and I have a hunch that it will be updated way more times than it has to. You can check this with React DevTools, but tl;dr, this should be reworked. It should be sufficient to only save the text to display along with the variant ('good' etc.).
- The
good
andbad
reducer actions do the exact same thing except set thegood
prop differently. This means that it should probably be generalized, and you should make anaddToast
action that takescontent
andvariant
.
I created a simple example of one way to solve snackbars. This handles more than one in a queue system, but reverting it to only storing one shouldn't be hard.
Now, how would you implement this in liquidator? Probably one of the easier ways would be to do the same as with auth
and transaction
-- i.e. create a context at the root that allows you to pass the dispatch down, allowing you to create snax from anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note related to the example;
If you opt for this method of handling state, remember to clear the timeout that removes the snack if the user clicks it. If not, if a similar toast appears later, it will be removed on the timeout of the first snack.
Right now:
- snack created; 'hello'
- user clicks; removed
- snack created; 'hello'
- timeout for first snack hits; removes second snack (3)
- timeout for second snack hits; does nothing
Implement snackbar feature when either adding transaction or trying to add transaction with name or description too long.
Closes #68
Closes #207