v2: select rewrite #362
Open
v2: select rewrite #362
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Changed panel ID references to content ID in Popover components for consistency. - Added aria-controls and aria-expanded attributes to PopoverTrigger for improved accessibility. - Introduced Select components (SelectContent, SelectRoot, SelectTrigger) that utilize Popover components, enhancing modularity. - Updated tests to ensure proper connection between trigger and content IDs, and verify aria attributes functionality.
- Introduced SelectRoot and SelectTrigger components, utilizing Popover for improved modularity. - Enhanced accessibility by adding aria-haspopup attribute to SelectTrigger. - Updated index export to include Select components. - Refactored Popover styles to remove unnecessary width properties.
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| ui-hover={context.hover} | ||
| ui-open={context.isOpen.value} | ||
| ui-closed={!context.isOpen.value} | ||
| {...props} |
There was a problem hiding this comment.
Bug: Hovered Popovers Fail Click Prevention
The handleClick function reads ui-hover from the trigger element, but this attribute was moved to the root element in this PR. The isHover check will always be false, breaking click prevention for hover-enabled popovers. The function should use context.hover from the popover context instead of reading the DOM attribute.
…ocumentation - Added new utility functions `getValueFromIndex` and `getIndexFromValue` to streamline item value management in the carousel. - Updated carousel components to utilize these utility functions for better performance and maintainability. - Expanded documentation to include additional component libraries and resources for contributors.
…oard interactions - Expanded Select component functionality by adding multiple item support and enhanced keyboard navigation. - Introduced typeahead functionality for quicker item selection. - Updated SelectItem and SelectTrigger components to improve accessibility and user experience. - Refactored SelectRoot to manage item states and selections more effectively.
…ement - Introduced SelectItem export in the index file for better component accessibility. - Refactored SelectItem to simplify index management by changing currItemIndex from a Signal to a regular number. - Updated SelectRoot to reflect the same change for improved state handling.
…ility for improved item management - Integrated registerItem utility in CarouselItem and SelectItem components to streamline item registration and state management. - Simplified index and value handling within both components for better maintainability. - Updated SelectTrigger to remove unnecessary click handling, enhancing clarity and functionality.
… components - Simplified key event handling in SelectContent, SelectItem, and SelectTrigger components for better readability and maintainability. - Enhanced conditional checks to prevent unnecessary operations when handling keyboard events. - Streamlined state updates for selected values and highlighted indices, improving overall user experience and accessibility.
- Added handleToggle function to manage focus on enabled items when the Select component is opened. - Implemented logic to focus the first enabled item if no item is highlighted or selected. - Updated SelectTrigger to remove unnecessary focus calls, streamlining keyboard interaction and improving accessibility.
- Removed redundant key event handling for Escape and Tab keys in SelectTrigger to simplify keyboard interactions. - Improved overall clarity and maintainability of the component's code.
| onPointerOut$={handlePointerOut$} | ||
| onPointerOver$={handlePointerOver$} | ||
| ui-open={isOpen.value} | ||
| ui-hover={hover} |
There was a problem hiding this comment.
Bug: Hover Mode: Pointer Events Go Awry
When hover mode is enabled, handlePointerMove$ incorrectly chains with props.onPointerOver$ instead of props.onPointerMove$. This causes any user-provided onPointerMove$ handler to be ignored in hover mode, while incorrectly calling the onPointerOver$ handler on pointer move events.
…ments - Updated SelectTrigger to include custom styling for better visual presentation. - Refactored SelectItem to utilize Select.ItemLabel for improved item labeling. - Added itemLabelRefsArray in SelectRoot for better item management and accessibility. - Exported new SelectItemLabel component for enhanced modularity and usability.
…ed labeling - Integrated useSignal for managing selected values in the Select component. - Updated SelectRoot to conditionally render a script for populating the value label based on selected items. - Added ui-qds-select-value-label attribute to SelectValueLabel for better integration with the Select component.
…item rendering - Added support for multiple Select components with individual selected values. - Refactored item rendering logic to dynamically generate Select.Item components from an array. - Improved styling consistency across Select components for better visual presentation.
…ed Select component code - Changed the default selected value from "3" to "5" in the basic example of the Select component. - Removed unused Select component code to streamline the example and improve clarity.
…le updates - Updated the basic example to set a new default selected value for the second Select component. - Introduced isDistinctValue state in SelectRoot to manage distinct selection logic. - Refactored context usage in SelectItemLabel for improved clarity and consistency. - Added SelectScript to SelectContent for enhanced functionality.
- Changed the default selected value in the basic example from "2" to "Item 2" for better clarity. - Introduced computed displayText in SelectValueLabel to handle multiple selections and improve label rendering. - Updated the return statement in SelectValueLabel to display the computed value, enhancing user experience.
| </Select.Item> | ||
| ))} | ||
| </Select.Content> | ||
| </Select.Root> |
There was a problem hiding this comment.
Bug: Select Component: Value Mismatch Causes Selection Error
The second Select.Root initializes selectedValueTwo with "Item 2" (a label), but the Select.Item components lack value props, causing auto-generated values like "0", "1", "2". When getIndexFromValue searches for "Item 2" in the values array, it won't find it and defaults to index 0, incorrectly selecting the first item instead of the second.
…pdates - Removed the default selected value in the basic example for improved clarity. - Updated SelectValueLabel to accept a placeholder prop, enhancing user experience when no value is selected. - Adjusted rendering logic in SelectValueLabel to display the placeholder when no selection is made.
…ample interactions - Introduced a button to toggle the rendering of a second Select component in the basic example, enhancing interactivity. - Updated SelectValueLabel to track display text for better handling of selected values. - Modified test cases to reflect changes in item labeling and selection behavior, ensuring consistency across examples. - Improved accessibility by updating placeholder text and ensuring proper aria attributes are set for items.
- Set the default selected value in the basic example to "6" for better clarity. - Removed the placeholder from Select.ValueLabel to enhance the display of selected options. - Refactored SelectValueLabel logic to improve handling of selected values and ensure proper rendering of labels.
…e display - Introduced a new ConditionalRender component to demonstrate dynamic rendering of the Select component. - Implemented a button to toggle the rendering of the Select, enhancing interactivity. - Updated test cases to verify that the correct label is displayed when the Select is conditionally rendered with an initial value.
| ); | ||
| }); | ||
|
|
||
| test.only("conditionally rendered select with initial value displays correct label", async () => { |
There was a problem hiding this comment.
Bug: Test marked with .only skips other tests
The test is marked with test.only which causes all other tests in the file to be skipped during test execution. This appears to be debugging code that was accidentally committed and will prevent the test suite from running properly in CI/CD pipelines.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note
Adds a new headless Select component with docs and tests, refactors Carousel to shared item/value utils, and improves Popover accessibility (aria attributes).
libs/components/src/select/*): New headlessSelectwithRoot,Trigger,Content,Item,ItemLabel,ValueLabel, and client script; supports keyboard navigation, typeahead, multi-select, disabled items, and controlledopen/valuewithonChange$.Selecttolibs/components/src/index.ts.registerItem(libs/utils/src/item-registration.ts) and item value helpers (item-value-utils.ts); re-exported inlibs/utils/src/index.ts.registerItemincarousel-item.tsx; update imports incarousel-*.tsx.*-content), setaria-controls/aria-expandedon trigger, pass props order updates; CSS anchor logic cleanup; add tests for aria linkage and state.docs/src/routes/components/select/*).Written by Cursor Bugbot for commit 47bad6a. This will update automatically on new commits. Configure here.