-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EuiSuperSelect] Fix various focus state behaviors #5097
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5097/ |
8aa4bb3
to
1782dd7
Compare
1782dd7
to
a6076bc
Compare
onFocus?: () => void; | ||
onBlur?: () => void; |
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.
This is my first time adding props to EUI, apologies if it's weird! I was copying EuiColorPicker
's typing here, since its behavior was what I wanted:
onFocus?: () => void; |
EuiComboBox
also (I believe) has similar onFocus/onBlur props that have no other comments/definitions (it does have an event handler, but we don't have the same event to hook into or pass back here):
eui/src/components/combo_box/combo_box.tsx
Line 103 in e8407ff
onFocus?: FocusEventHandler<HTMLDivElement>; |
Based on the above 2 components, I was guessing that I didn't need additional props documentation for onFocus
/onBlur
(which we mainly only expect to get via parent EuiFormRow
s, but if I'm missing something else here, definitely let me know!
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.
Correct, it doesn't make sense to add documentation for these.
I'm wondering if each should accept an optional event parameter, though. Both of these will get passed via rest
to the EuiSuperSelectControl
(L287), and in that case would be provided an event.
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.
Unfortunately in a few cases (e.g. on popover close), there might not be a relevant event to pass (and also the typing might end up being a headache 😅)
Just to confirm, would you be fine merging this as-is (since this already exists in EuiColorPicker) and addressing events as a separate tech debt issue/PR?
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.
onFocus?: () => void; | |
onBlur?: () => void; | |
onFocus?: (event?: FocusEvent) => void; | |
onBlur?: (event?: FocusEvent) => void; |
Should be enough from a type perspective, after importing FocusEvent
from react
.
Also, onFocus
and onBlur
should be included in the Omit
statement on L36 because these new definitions are overriding those from EuiSuperSelectControlProps
.
I'd like to include these now rather than later because technically it would be a breaking change to remove the event params, which are there by default from EuiSuperSelectControlProps
(from <ButtonHTMLAttributes<HTMLButtonElement>
), and would cause errors if a consumer already has a handler in place.
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.
Sounds good to me - just to confirm, are you asking for type changes only, or should I be endeavoring to actually pass in events into the props.onFocus()
and props.onBlur()
calls?
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.
1e1adb5 - let me know if I missed anything, or if there's anything else to change!
Preview documentation changes for this PR: https://eui.elastic.co/pr_5097/ |
This looks awesome, thanks! Quick question - Which version will it come out on, or will it be backported to previous versions? |
@brittanyjoiner15 Most likely included in eui@37.4.0 and will make it into 7.16. It would be possible to backport to 7.15 if required. |
thanks @thompsongl ! |
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.
Took another pass. To avoid a breaking change, we'll want to make a couple changes.
onFocus?: () => void; | ||
onBlur?: () => void; |
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.
onFocus?: () => void; | |
onBlur?: () => void; | |
onFocus?: (event?: FocusEvent) => void; | |
onBlur?: (event?: FocusEvent) => void; |
Should be enough from a type perspective, after importing FocusEvent
from react
.
Also, onFocus
and onBlur
should be included in the Omit
statement on L36 because these new definitions are overriding those from EuiSuperSelectControlProps
.
I'd like to include these now rather than later because technically it would be a breaking change to remove the event params, which are there by default from EuiSuperSelectControlProps
(from <ButtonHTMLAttributes<HTMLButtonElement>
), and would cause errors if a consumer already has a handler in place.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5097/ |
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.
just to confirm, are you asking for type changes only, or should I be endeavoring to actually pass in events into the props.onFocus() and props.onBlur() calls?
Type changes only; no need to pass a manufactured event 👍
- to allow for arrow key UX
- by manually triggering the onFocus passed by EuiFormRow that determines the label's focus state - onFocus and onBlur types are based off of ColorPicker
1ba3b9c
to
0176f31
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_5097/ |
Summary
9566eed closes #5091 (up/down arrow keys don't work when an initial value is not passed)
c314a99 closes #5094 (EuiFormRow labels don't show as focused when interacting with the dropdown)
Screencap of fixed behavior:
A screencap of keyboard behavior isn't the best though, so it's probably easier to follow the QA/repro steps below:
QA
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Checked in Chrome, Safari, Edge, and Firefox- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Checked Code Sandbox works for the any docs examples- [ ] Added or updated jest tests- [ ] Checked for breaking changes and labeled appropriately