Skip to content
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.

Sign up for GitHub

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

fix: Pass value to visual input in select #2676

Merged
merged 5 commits into from
Apr 9, 2024
+24 −8
Merged

fix: Pass value to visual input in select #2676

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f27b605
fix: Pass value to visual input in select
Apr 3, 2024
99220f7
fix: Update select
Apr 3, 2024
c66adc2
fix: Remove consoles
Apr 3, 2024
181cf56
fix: Add logic back in
Apr 3, 2024
171a0b3
Simplified logic
NicholasBoll Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion modules/react/select/lib/hooks/useSelectInput.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {useSelectModel} from './useSelectModel';
*/
export const useSelectInput = composeHooks(
createElemPropsHook(useSelectModel)(
(model, ref, elemProps: {keySofar?: string; placeholder?: string} = {}) => {
(model, ref, elemProps: {keySofar?: string; placeholder?: string; value?: string} = {}) => {
const {elementRef} = useLocalRef<HTMLInputElement>(ref as any);
const textInputRef = React.useRef<HTMLInputElement>(null);

Expand All @@ -30,6 +30,7 @@ export const useSelectInput = composeHooks(
Object.getPrototypeOf(textInputRef.current),
'value'
);

if (nativeInputValue && nativeInputValue.set) {
nativeInputValue.set.call(textInputRef.current, value);
}
Expand All @@ -54,6 +55,7 @@ export const useSelectInput = composeHooks(
model.state.selectedIds[0]
) {
const value = model.navigation.getItem(model.state.selectedIds[0], model).id;

if (
model.state.selectedIds[0] !== value &&
model.state.inputRef.current.value !== value
Expand Down Expand Up @@ -130,6 +132,11 @@ export const useSelectInput = composeHooks(
},
textInputProps: {
ref: textInputRef,
value: elemProps.value
? model.navigation.getItem(model.state.selectedIds[0], model).textValue
: model.state.selectedIds.length > 0 && model.state.items.length > 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: model.state.selectedIds.length > 0 && model.state.items.length > 0
: model.state.selectedIds.length && model.state.items.length

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the failure of the ternary supposed to do? The first part says "if the developer passes a value, do a text lookup of that value". The original sandbox had the failure case return undefined, meaning "if the developer didn't pass a value, don't treat this like a controllable input". But the model.state.selectedIds.length > 0 && model.state.items.length > 0 will return true or false if a value is not provided. What does this do? It doesn't match the original intent, but what is the side effect?

? model.navigation.getItem(model.state.selectedIds[0], model).textValue
: undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need someone to verify this logic. When Nicholas worked on this in this sandbox it worked. However, it broke the dynamic items story because it wasn't setting the value of the visual input.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also this change now throws the following error because we're updating the value ourselves:

Warning: You provided a `value` prop to a form field without an `onChange` handler. This will render a read-only field. If the field should be mutable use `defaultValue`. Otherwise, set either `onChange` or `readOnly`.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment splitting the code gave me trouble trying to understand the logic in its entirety:

          value: elemProps.value
            ? model.navigation.getItem(model.state.selectedIds[0], model).textValue
            : model.state.selectedIds.length > 0 && model.state.items.length > 0
            ? model.navigation.getItem(model.state.selectedIds[0], model).textValue
            : undefined,

It looks like this code says the following: "if the developer passed a value, look up the text value of the id. If not, see if there is selected ids and items, if yes, look up the text value, otherwise send undefined".

This doesn't make sense. Are you meaning it to always be a "controlled" input to React? It seems like the following would work better:

          model.state.selectedIds.length > 0 && model.state.items.length > 0
            ? model.navigation.getItem(model.state.selectedIds[0], model).textValue
            : undefined,

This logic says if there's a selected id and items, the value of the visual input should be the text value of the server value. The test for model.state.items is to make sure the getItem call doesn't fail on an error.

},
ref: elementRef,

Expand Down
Loading