-
Notifications
You must be signed in to change notification settings - Fork 336
refactor(core): update state reducer signature #337
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,40 +1,40 @@ | ||
| import { Reducer } from './types'; | ||
| import { getItemsCount, getNextHighlightedIndex } from './utils'; | ||
|
|
||
| export const stateReducer: Reducer = (action, state, props) => { | ||
| export const stateReducer: Reducer = (state, action) => { | ||
| switch (action.type) { | ||
| case 'setHighlightedIndex': { | ||
| return { | ||
| ...state, | ||
| highlightedIndex: action.value, | ||
| highlightedIndex: action.payload, | ||
| }; | ||
| } | ||
|
|
||
| case 'setQuery': { | ||
| return { | ||
| ...state, | ||
| query: action.value, | ||
| query: action.payload, | ||
| }; | ||
| } | ||
|
|
||
| case 'setSuggestions': { | ||
| return { | ||
| ...state, | ||
| suggestions: action.value, | ||
| suggestions: action.payload, | ||
| }; | ||
| } | ||
|
|
||
| case 'setIsOpen': { | ||
| return { | ||
| ...state, | ||
| isOpen: action.value, | ||
| isOpen: action.payload, | ||
| }; | ||
| } | ||
|
|
||
| case 'setStatus': { | ||
| return { | ||
| ...state, | ||
| status: action.value, | ||
| status: action.payload, | ||
| }; | ||
| } | ||
|
|
||
|
|
@@ -43,7 +43,7 @@ export const stateReducer: Reducer = (action, state, props) => { | |
| ...state, | ||
| context: { | ||
| ...state.context, | ||
| ...action.value, | ||
| ...action.payload, | ||
| }, | ||
| }; | ||
| } | ||
|
|
@@ -55,7 +55,7 @@ export const stateReducer: Reducer = (action, state, props) => { | |
| 1, | ||
| state.highlightedIndex, | ||
| getItemsCount(state), | ||
| props.defaultHighlightedIndex | ||
| action.props.defaultHighlightedIndex | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a reason these variables are nested in action.props, and not top-level in action, for an easier type signature for action?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI Did you mean to not retrieve this value from
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't get that props meant options, it makes sense in that case then! |
||
| ), | ||
| }; | ||
| } | ||
|
|
@@ -67,7 +67,7 @@ export const stateReducer: Reducer = (action, state, props) => { | |
| -1, | ||
| state.highlightedIndex, | ||
| getItemsCount(state), | ||
| props.defaultHighlightedIndex | ||
| action.props.defaultHighlightedIndex | ||
| ), | ||
| }; | ||
| } | ||
|
|
@@ -108,8 +108,10 @@ export const stateReducer: Reducer = (action, state, props) => { | |
|
|
||
| // Since we close the menu when openOnFocus=false | ||
| // we lose track of the highlighted index. (Query-suggestions use-case) | ||
| props.openOnFocus === true ? props.defaultHighlightedIndex : null, | ||
| isOpen: props.openOnFocus, // @TODO: Check with UX team if we want to close the menu on reset. | ||
| action.props.openOnFocus === true | ||
| ? action.props.defaultHighlightedIndex | ||
| : null, | ||
| isOpen: action.props.openOnFocus, // @TODO: Check with UX team if we want to close the menu on reset. | ||
| status: 'idle', | ||
| statusContext: {}, | ||
| query: '', | ||
|
|
@@ -119,13 +121,13 @@ export const stateReducer: Reducer = (action, state, props) => { | |
| case 'focus': { | ||
| return { | ||
| ...state, | ||
| highlightedIndex: props.defaultHighlightedIndex, | ||
| isOpen: props.openOnFocus || state.query.length > 0, | ||
| highlightedIndex: action.props.defaultHighlightedIndex, | ||
| isOpen: action.props.openOnFocus || state.query.length > 0, | ||
| }; | ||
| } | ||
|
|
||
| case 'blur': { | ||
| if (props.debug) { | ||
| if (action.props.debug) { | ||
| return state; | ||
| } | ||
|
|
||
|
|
@@ -139,14 +141,14 @@ export const stateReducer: Reducer = (action, state, props) => { | |
| case 'mousemove': { | ||
| return { | ||
| ...state, | ||
| highlightedIndex: action.value, | ||
| highlightedIndex: action.payload, | ||
| }; | ||
| } | ||
|
|
||
| case 'mouseleave': { | ||
| return { | ||
| ...state, | ||
| highlightedIndex: props.defaultHighlightedIndex, | ||
| highlightedIndex: action.props.defaultHighlightedIndex, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eg. this could maybe just be the payload?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, merging as is then! |
||
| }; | ||
| } | ||
|
|
||
|
|
||
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.
was this never used?
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.
Nope. I initially supported Shift↓ to jump items 5 by 5 but then decided that it was gadget. I forgot to remove this parameter.