-
Notifications
You must be signed in to change notification settings - Fork 843
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
Spied input value as a state variable in EuiFieldSearch #3270
Conversation
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3270/ |
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.
After clicking the 'x' clear button, the text does get removed, but the button remains. The button should also hide after it is clicked.
fixed the issue |
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.
Two more cases where the clear button gets out of sync with the input value:
- When an empty input has its
value
set dynamically, no clear button is shown - When an input with a
value
set, and thatvalue
is then set to''
dynamically, the clear button remains visible.
I tried both the cases that you mentioned by changing this to set the value to
Can you give a code overview and let me know if i am doing something wrong? |
The value updates I mentioned happen when the export default () => {
const [value, setValue] = useState('');
const onChange = e => {
setValue(e.target.value);
};
return (
<>
<button onClick={() => setValue('outside')>Set to outside</button>
<EuiFieldSearch
value={value}
onChange={onChange}
isClearable={true}
/>
</>
)
} Clicking the button will result in the prop value and state value of |
Sir, since the problem arises when no |
jenkins test this The latest update appears to fix the issue case and the regressions introduced moving from props to state. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3270/ |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3270/ |
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.
Thanks for working through this, @anishagg17!
Summary
Fixes: #2860
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Checked in IE11 and Firefox- [ ] Added documentation examples- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes