[EuiInputPopover] Refactor & clean ups #7241
Conversation
cee-chen
force-pushed
the
input-popover-setup
branch
from
0c63c75
to
e8ca2d6
Compare
- allows us to remove an extra unnecessary div wrapper, and it general lends itself to cleaner and more React-like code + add more detailed/clearer tests to reflect the comments added to the useEffects
- [microperf] don't bother finding/iterating through tabbable items when `disableFocusTrap` is true - [microperf] Clean up unnecessary extra Array map by using `.hasAttribute()` instead - make if conditions more readable/understandable by using early returns, var names, and writing tests - pull out `closePopover` from props to add it to dependency array - remove unnecessary type
- Remove unnecessary div by passing onKeyDown to the parent EuiPanel popover - now that the current target of the keydown event can be guaranteed to be the panel, we can remove the `panelEl` dependency from the array
- Move classes/styling up to top of file - add jsdoc block - add clearer comments on why state is being used over refs for the input/panel refs
cee-chen
force-pushed
the
input-popover-setup
branch
from
e8ca2d6
to
98805e0
Compare
|
Preview staging links for this PR:
|
💚 Build Succeeded
|
|
@breehall Any chance I can shoot this PR your way today/tomorrow for review? 🙏 It's somewhat high priority as it's blocking another EuiComboBox PR. If you don't have bandwidth, no worries, please let me know! |
|
Absolutely! Taking a look now |
There was a problem hiding this comment.
This is good to go! This was a good refactor with great code quality improvements around comments, readability, and code structure. Great work simplifying the portion of code used to set width on the popover and removing an unnecessary .map(). I tested the production and staging versions of the PR and don't see any changes visually or in functionality.
| const inputWidth = useResizeObserver(inputEl, 'width').width; | ||
|
|
||
| const panelWidth = useMemo(() => { | ||
| return inputWidth < panelMinWidth ? panelMinWidth : inputWidth; | ||
| }, [panelMinWidth, inputWidth]); |
There was a problem hiding this comment.
[Attaching to the closest block of code] This is a lot easier to understand (especially with the new comments)
There was a problem hiding this comment.
Music to my ears. Thank you so much Bree!
Also, for some context as to what I was cursing about & struggling with that I mentioned earlier today - it took me way too long to figure out what the tabbable logic was trying to accomplish and how to QA it (it really seems, functionally, to only affect the EuiAutoRefresh component). Which is why writing tests and leaving good code comments is so important :)
Summary
I'm working on pulling out some logic that
EuiComboBoxneeds directly intoEuiInputPopoverfor #7192, and was struggling to understand and work around some of the existing logic. Since I saw several things that bothered me/that could stand to be cleaned up, I decided to do my favorite thing - refactor and write missing tests 😅I've opted to make this refactor separate/standalone from my
EuiComboBoxwork in order to make code review less of a headache. As always, I strongly recommend following along by commit and hiding whitespace changes (the scary LOC diffs on this PR are literally just from indentation changes on snapshots).What this PR does:
<div>in the popoverinline-sizeproperty instead ofwidthonKeydowncallbackEuiInputPopovertests to automatically catch if code changes/breaks something that was meant to do a specific thing a specific wayQA
Functionally, nothing should have changed in this PR except for 2 removed
<div>wrappers and a logical property.General checklist
and screenreader modes- [ ] Checked in both light and dark modesjest andcypress tests