-
Notifications
You must be signed in to change notification settings - Fork 17
fix(picker): prevent clearing input when chip menu opens #3849
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -547,9 +547,40 @@ export class Picker { | |
| return; | ||
| } | ||
|
|
||
| // Don't clear if a chip's menu is open - focus moved to the menu's portal | ||
| if (this.hasOpenChipMenu()) { | ||
| return; | ||
| } | ||
|
Comment on lines
+550
to
+553
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After the chip menu closes, no mechanism triggers When Consider listening for the chip menu's close and calling 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at this more carefully, I believe the behavior is consistent and correct:
When the menu closes and the user later refocuses on the picker:
This is consistent, expected behavior — the user's search is preserved across the menu interaction, which aligns with the The concern would be valid if the visual input got cleared but |
||
|
|
||
| this.clearInputField(); | ||
| } | ||
|
|
||
| private hasOpenChipMenu(): boolean { | ||
| if (!this.chipSet) { | ||
| return false; | ||
| } | ||
|
|
||
| // Query for any open menus inside the chips | ||
| const chips = this.chipSet.shadowRoot?.querySelectorAll('limel-chip'); | ||
| if (!chips) { | ||
| return false; | ||
| } | ||
|
|
||
| for (const chip of chips) { | ||
| // Check the `open` property directly rather than using an attribute | ||
| // selector, because Stencil batches attribute reflection and | ||
| // the [open] attribute may not be on the DOM yet | ||
| const menu = chip.shadowRoot?.querySelector( | ||
| 'limel-menu' | ||
| ) as HTMLLimelMenuElement; | ||
| if (menu?.open) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
Comment on lines
+558
to
+582
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial 🧩 Analysis chain🏁 Script executed: # Find the limel-menu component
fd 'menu.tsx' --type f | head -20Repository: Lundalogik/lime-elements Length of output: 622 🏁 Script executed: # Find the limel-chip-set component
fd 'chip-set.tsx' --type f | head -20Repository: Lundalogik/lime-elements Length of output: 150 🏁 Script executed: # Look at the limel-menu component to check if 'open' is reflected
rg -n "@Prop" --type ts -A 5 | grep -A 5 "open"Repository: Lundalogik/lime-elements Length of output: 6974 🏁 Script executed: # Check limel-chip-set component
cat -n src/components/chip-set/chip-set.tsx | head -100Repository: Lundalogik/lime-elements Length of output: 4114 🏁 Script executed: # Check limel-chip component
fd 'chip.tsx' --type f | grep -v examples | grep -v chip-setRepository: Lundalogik/lime-elements Length of output: 96 🏁 Script executed: # Check if there are any similar shadow DOM traversals in the codebase
rg "shadowRoot\?" --type ts -B 2 -A 2 | head -80Repository: Lundalogik/lime-elements Length of output: 4474 🏁 Script executed: # Check if chip-set or chip have any public methods for checking menu state
rg "@Method()" src/components/chip-set/chip-set.tsx src/components/chip/chip.tsxRepository: Lundalogik/lime-elements Length of output: 220 🏁 Script executed: # Check the full chip-set to see if there's any method that could help
wc -l src/components/chip-set/chip-set.tsxRepository: Lundalogik/lime-elements Length of output: 108 🏁 Script executed: # Check chip component for menu-related functionality
cat -n src/components/chip/chip.tsx | head -150Repository: Lundalogik/lime-elements Length of output: 5914 🏁 Script executed: # Check if chip has any public methods
rg "@Method()" src/components/chip/chip.tsxRepository: Lundalogik/lime-elements Length of output: 50 🏁 Script executed: # Look at where limel-menu is rendered in limel-chip
rg -n "limel-menu" src/components/chip/chip.tsx -B 2 -A 2Repository: Lundalogik/lime-elements Length of output: 343 🏁 Script executed: # Get a sense of the full context - look at the complete chip render to understand the structure
sed -n '200,400p' src/components/chip/chip.tsxRepository: Lundalogik/lime-elements Length of output: 5953 🏁 Script executed: # Check if there are any similar patterns checking menu.open state in the codebase
rg "menu\[open\]|\.open" src/components/chip/chip.tsx src/components/picker/picker.tsx -B 2 -A 2Repository: Lundalogik/lime-elements Length of output: 399 🏁 Script executed: # Check what the three `@Method` decorators are in chip-set
rg -n "@Method()" src/components/chip-set/chip-set.tsx -A 3Repository: Lundalogik/lime-elements Length of output: 442 The shadow DOM traversal pattern is used elsewhere in the codebase, but consider refactoring for better encapsulation. This method reaches into multiple shadow DOM boundaries ( Note: The 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion! You're right that encapsulating this in Adding a public |
||
|
|
||
| /** | ||
| * Input handler for the input field | ||
| * | ||
|
|
||
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.
In its current state, this PR does not solve the problem. Here is a short screen-recording showing the problem still exists.
limel-picker.with.chips.with.menu.mov
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.
Good catch! The issue was that I was using an attribute selector (
limel-menu[open]) to detect an open menu, but Stencil batches attribute reflection with the render cycle. By the timehasOpenChipMenu()runs, the menu'sopenproperty is alreadytrue, but the[open]attribute hasn't been reflected to the DOM yet.Fixed in ⚡ eb4f20e by checking the property directly instead: