refactor(core): update state reducer signature #337
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4606a7a:
|
| // Arrow down. | ||
| event.preventDefault(); | ||
|
|
||
| store.send(event.key, { shiftKey: event.shiftKey }); |
There was a problem hiding this comment.
Nope. I initially supported Shift↓ to jump items 5 by 5 but then decided that it was gadget. I forgot to remove this parameter.
| state.highlightedIndex, | ||
| getItemsCount(state), | ||
| props.defaultHighlightedIndex | ||
| action.props.defaultHighlightedIndex |
There was a problem hiding this comment.
is there a reason these variables are nested in action.props, and not top-level in action, for an easier type signature for action?
There was a problem hiding this comment.
FYI defaultHighlightedIndex is an Autocomplete option.
Did you mean to not retrieve this value from props but rather from payload.defaultHighlightedIndex?
There was a problem hiding this comment.
I didn't get that props meant options, it makes sense in that case then!
| return { | ||
| ...state, | ||
| highlightedIndex: props.defaultHighlightedIndex, | ||
| highlightedIndex: action.props.defaultHighlightedIndex, |
There was a problem hiding this comment.
eg. this could maybe just be the payload?
There was a problem hiding this comment.
In my mind it's still not totally clear what should be passed in our payload when thinking of it as an API. Can you explain the reasoning behind your suggestion?
There was a problem hiding this comment.
I don't really use the reducer pattern so often, but saw action.type as the identifier, and then in the cases itself, I usually only saw payload being used, so when writing the dispatch, you don't have to think whether to use payload or props or something else.
Props of course doesn't make sense to think about, since those are the "global arguments", which makes sense as its own thing indeed
There was a problem hiding this comment.
Yep, merging as is then!
This simplifies the signature of the store and the reducer, to be more conformed to the state reducer pattern (see React's
useReduceras example):this.stateand relies on scopedstatevariable, non-accessible from the outside(state, action) => state{ type, payload, props }where props are the options passed to AutocompleteThis standardizes the state reducer signature if we were to make this a public API later.