Firefly-2087: Refactor SuggestBoxInputField to Use a Functional Component - #2004
Firefly-2087: Refactor SuggestBoxInputField to Use a Functional Component#2004aventura121 wants to merge 9 commits into
Conversation
The component-library storybook build currently fails on an unrelated circular-import fault in ChartPanel, so this story cannot be rendered or verified. Re-add once that is fixed. The index.js exports stay, since they are the published package surface, not storybook.
aventura121
changed the title
aventura121
changed the title
loitly
left a comment
loitly
left a comment
There was a problem hiding this comment.
Good work on refactoring the component. However, I have concern about the multiple validators. Could you review it and see if my concern is valid.
| // pre-joy version: it tests every ';' token, not just the last one being typed. | ||
| function ucdFilterOptions(options, {inputValue}) { | ||
| // joy passes an empty inputValue on the render that opens the popup, so returning [] here would | ||
| // blank the listbox on the first keystroke |
There was a problem hiding this comment.
The comments should describe the code itself, not the changes made for this PR. This becomes irrelevant later when reviewing the code.
For example, the comment could be:
// UCD1+ value is a ‘;’ delimited list. The filter should return options that match any term.
// When the input is empty, return all options.
| value: any, | ||
| valid: bool, | ||
| message: string, | ||
| tooltip: string, | ||
| showWarning: bool, | ||
| validator: func, // (value) => ({valid, message}), run on every keystroke and on a selection |
There was a problem hiding this comment.
Promoted to top-level props makes sense here.
| initialState: shape({ | ||
| value: string, | ||
| valid: bool, | ||
| message: string, | ||
| validator: func, | ||
| nullAllowed: bool, | ||
| }), | ||
| ...omit(AutoCompleteInputView.propTypes, 'value', 'valid', 'message', 'fireValueChange'), |
There was a problem hiding this comment.
Folding the promoted view's props back into initialState to preserve exiting API convention is fine. But, validator is missing which allow this component to have 2 separate validators. There should only be one shared by both the view and the FieldGroup controller.
| export function useAsyncOptions(getOptions, value) { | ||
| const [result, setResult]= useState({options:[], loading:false}); |
There was a problem hiding this comment.
useDebounced and useAsyncOptions are great additions. They fix existing bugs. I think these functions can be reused as is or with minor tweak. If you’re interested in taking on the task, that’s something to consider.
2 participants
Firefly-2087: Refactor SuggestBoxInputField to Use a Functional Component
Description
This PR removes all of the callers to SuggestBoxInput to the Joy UI component, AutoComplete/AutoCompleteView.
Companion PR in irsa-ife (to be merged after): https://github.com/IPAC-SW/irsa-ife/pull/477
Testing
Firefly build: https://firefly-2087-use-joy-autocomplete-all.irsakubedev.ipac.caltech.edu/firefly
Irsa-ife-sha build (SHA needs changes related to this PR): https://firefly-2087-use-joy-autocomplete-all.irsakubedev.ipac.caltech.edu/applications/Spitzer/SHA
Irsa-ife-wise build (wise has no irsa-ife specifc code paths related to this PR): https://firefly-2087-use-joy-autocomplete-all.irsakubedev.ipac.caltech.edu/applications/wise
For firefly:
Screenshot
For irsa-ife-sha (contains irsa-ife specific code changes):
Screenshot
For irsa-ife-wise:
Screenshot