Add post-launch tweaks #10
Conversation
|
TS PR: reduxjs/redux-templates#6 |
| } | ||
| > | ||
| Add Amount | ||
| </button> | ||
| </div> | ||
| <div className={styles.row}> |
There was a problem hiding this comment.
The layout here feels a bit confusing to me after looking at the gif, in a couple ways:
- The buttons aren't aligned
- It's surprising to me that "Add Async" doesn't add the amount in the textbox
Can we put the textbox and both buttons in a single row, and make this do dispatch(incrementAsync(incrementAmount) ? That also lets us show passing an argument to a thunk while we're at it.
There was a problem hiding this comment.
Actually, given that, would it be better to store the amount as a number permanently, and do the number conversion in the onChange handler?
There was a problem hiding this comment.
I think it will lead to a simpler example if we leave it stored as a string. Storing as a number means we'd be doing a form of input masking, which requires more code to make work properly for things like "", "1.", "001", and other intermediate forms of number inputs. I talked a little more about this in reduxjs/redux-templates#3 (comment)
| export const { increment, decrement, incrementByAmount } = slice.actions; | ||
|
|
||
| export const incrementAsync = () => dispatch => { |
There was a problem hiding this comment.
Let's add a comment here saying that this is a thunk, and you can do async logic inside.
There was a problem hiding this comment.
I took a shot at adding a comment. Let me know if you want different wording
|
Requested tweaks apply to the TS side as well. (Also, neat animation on the button!) |
|
@markerikson Made those changes. If they're all good I'll make the same to the TS PR. |
|
I like it. Let's do this. Go ahead and update the TS repo accordingly. Thanks! |
Implements changes described in #7 and reduxjs/redux-templates#3 (comment)