-
Notifications
You must be signed in to change notification settings - Fork 223
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
fix: Pass value to visual input in select #2676
Conversation
Passing run #7141 ↗︎Details:
Review all test suite changes for PR #2676 ↗︎ |
? 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, |
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.
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.
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.
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`.
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.
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.
@@ -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 |
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.
: model.state.selectedIds.length > 0 && model.state.items.length > 0 | |
: model.state.selectedIds.length && model.state.items.length |
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.
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?
Summary
value
was not passed to the underlying select input. Consumers would passvalue
but it wasn't working. This change passes the value to the input so consumers can control it and the visual input would reflect it..Release Category
Components
Checklist
ready for review
has been added to PRFor the Reviewer
Where Should the Reviewer Start?
Areas for Feedback? (optional)
Testing Manually
Screenshots or GIFs (if applicable)
Thank You Gif (optional)