Make Select components clearable by default
#478
Conversation
This Hook reverted a breaking change introduced in React Select v3 while Styled UI was still in v5. There's a TODO note on it to remove the Hook in v6, and we're already warning about this breaking change in the v6 upgrade guide, so I think it best to remove it now. This isn't essential to this PR, though. If this commit is merged, I'll post a special notice in the Styled UI flow.
FYI, we have a PR build environment 😄 |
|
Oh that's much better! 😅 |
Four components was getting to be a lot to keep prop transformation logic in sync between, especially as I'm about to add a Hook to all of them that uses seven more props.
|
I've added functionality to clear the |
|
This is ready for another look, by the way, @RobertBolender 👍🏼 |
There was a problem hiding this comment.
I'm not totally convinced that we want to support the optionally-controlled-state pattern/hook long-term, but I'm a big fan of the useCommonSelectProps cleanup and am very happy with the UX improvement with clearing the Single Select when typing.
Let's be sure to merge this with Rebase, not Squash, to preserve the individual commits.
| [innerOnSelectChange, isClearable, isMulti, onInputChange, setInputValue], | ||
| ); | ||
|
|
||
| return [selectValue, innerOnSelectChange, inputValue, innerOnInputChange]; |
There was a problem hiding this comment.
Nit (non-blocking): I realize this hook is pretty tightly coupled to the selects so it's not a huge deal although can we return an object here? My general rule of thumb is to prefer returning objects from hooks when
- There are more than two values being returned
- The hook is likely to be called once and only once in a component
My reasoning for this is so that A) consumers are encouraged to use consistent variable names and B) consumers don't have to worry about getting the order correct when destructuring
| const [value, setValue] = useState( | ||
| controlledValue !== undefined ? controlledValue : defaultValue, | ||
| ); | ||
| useEffect(() => { |
There was a problem hiding this comment.
Nit (non-blocking): I think this hook could be simplified a bit
const isControlled = controlledValue !== undefined;
const [value, setValue] = useState(isControlled ? controlledValue : defaultValue);
if (isControlled) {
return [controlledValue, EMPTY_FUNCTION];
}
return [value, setValue];There was a problem hiding this comment.
This way saves you a useEffect call and a few lines of code
There was a problem hiding this comment.
Nice utility hook by the way!
There was a problem hiding this comment.
Oh yeah, I suppose that useEffect is a bit useless there. 😅 Thanks! I think I'll simplify it a bit more to
const [value, setValue] = useState(defaultValue);
if (controlledValue !== undefined) {
return [controlledValue, EMPTY_FUNCTION];
} else {
return [value, setValue];
}then, since value and setValue won't be used if controlledValue's defined.
|
As we'll be rebasing instead of squashing for this one, I'll edit the last commit instead of adding another so the git history's more useful. |
| return { | ||
| innerValue: selectValue, | ||
| innerOnChange: innerOnSelectChange, | ||
| innerInputValue: inputValue, | ||
| innerOnInputChange, | ||
| }; |
There was a problem hiding this comment.
@josephdangerstewart and @RobertBolender, the new return object for useClearableSelectValue. Involves some name changes that make more sense outside the Hook.
| const [value, setValue] = useState(defaultValue); | ||
|
|
||
| if (controlledValue !== undefined) { | ||
| return [controlledValue, EMPTY_FUNCTION]; | ||
| } else { | ||
| return [value, setValue]; | ||
| } |
There was a problem hiding this comment.
@josephdangerstewart, the simplified useOptionallyControlledState body.
Only for clearable single-value Selects.
FLCOM-6739
It was noted as confusing that clearing a
Selectcomponent leaves an old value in the input. After investigating the React Select docs, I'm guessing that such components weren't clearable at all, since itsSelectcomponents have anisClearableprop that is falsy by default.This PR adds an explicit
isClearableprop to all Styled UISelectcomponents and gives it a default value oftrue.Probably the easiest way to test this would be to
check out the branch, thengo to https://deploy-preview-478--faithlife-styled-ui.netlify.app/#/text-input/select.yarn && yarn build && yarn catalog-start, then go to http://localhost:4000/#/text-input/select