add Notification Type to Snack Messaging #2884
Conversation
|
@fabian-bizfactory please let me know once you've looked through the diff and you think this is ready for review |
|
Sorry, about the PR pull. Found a potential issue after creating it |
| const resetMessage = () => dispatch(setMessage({ message: null })); | ||
| const resetMessage = () => | ||
| dispatch( | ||
| setMessage({ |
There was a problem hiding this comment.
Let's use the resetMessage action instead here
| @@ -4,12 +4,19 @@ import { Action } from "../app/actions"; | |||
|
|
|||
| import { resetMessage, setMessage } from "./actions"; | |||
|
|
|||
| export enum SnackMessageTypeT { | |||
There was a problem hiding this comment.
Ending types with aT is something we're trying to slowly move away from, I'd suggest calling it SnackMessageType
|
|
||
| const Root = styled("div")` | ||
| const colorLookupTable: { [key in SnackMessageTypeT]: string } = { |
There was a problem hiding this comment.
Let's avoid calling variables by their "data structure" ("lookup table") and instead describe what they contain. Also Record is useful here:
| const colorLookupTable: { [key in SnackMessageTypeT]: string } = { | |
| const bgColorByType: Record<SnackMessageType, string> = { |
| export type SnackMessageActions = ActionType< | ||
| typeof setMessage | typeof resetMessage | ||
| >; | ||
|
|
||
| export const setMessage = createAction("snack-message/SET")<{ | ||
| message: string | null; | ||
| notificationType: SnackMessageTypeT; |
There was a problem hiding this comment.
how about just calling it type?
| notificationType: SnackMessageTypeT; | |
| type: SnackMessageType; |
| const Root = styled("div")` | ||
| const colorLookupTable: { [key in SnackMessageTypeT]: string } = { | ||
| [SnackMessageTypeT.ERROR]: "rgba(0, 0, 0, 0.75)", | ||
| [SnackMessageTypeT.SUCCESS]: "rgba(12, 100, 39, 0.75)", // #0C6427 |
There was a problem hiding this comment.
Have you tested what it looks like? I think with the green, it's actually too transparent, how about using 0.9?
There was a problem hiding this comment.
I liked it that way. Took a look at 0.9, you are right, that looks better
No description provided.