chore: update select component #38954
Conversation
github-actions
bot
added
the
skip-changelog
WalkthroughThis update enhances the Select component by adding grouped options with checkboxes via a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Select as Select Component
participant RCSelect
participant SearchInput
User->>Select: Clicks dropdown
Select->>RCSelect: onDropdownVisibleChange(open)
RCSelect->>Select: Render dropdown & trigger dropdownRender
Select->>SearchInput: Focus search input (if showSearch)
User->>SearchInput: Types search term
SearchInput-->>Select: Filter options
Select->>User: Display grouped options with checkboxes via OptGroup
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)`app/client/cypress/**/**.*`: Review the following e2e test ...
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app/client/packages/design-system/ads/src/Select/rc-styles.css (1)
121-130: Multi‑Select Search Mirror: Invalid CSS Property
The styles for.rc-select-multiple .rc-select-selector .rc-select-selection-search-mirror(lines 121–130) correctly position the element; however, the declarationposition: none;(line 125) is invalid CSS. You should remove or correct this property. For example:- position: none;This change is critical to avoid unexpected behavior in browsers.
🧹 Nitpick comments (8)
app/client/packages/design-system/ads/src/Select/Select.tsx (2)
69-82: Consider memoizing the dropdownRender callback.The dropdown render function is recreated on every render. Consider using useCallback to optimize performance.
+const dropdownRender = useCallback((menu: React.ReactNode) => { + return ( + <div> + {showSearch && ( + <SearchInput + onChange={setSearchValue} + placeholder="Type to search..." + size="md" + /> + )} + {menu} + </div> + ); +}, [showSearch]);
98-110: Add type safety to tagRender function.The function uses implicit any types. Consider adding proper type definitions.
-tagRender={(props) => { +tagRender={(props: { closable: boolean; label: React.ReactNode; onClose: () => void }) => {app/client/packages/design-system/ads/src/Select/styles.css (1)
309-313: Update Option Hover Styling
Enforcing the hover background color via!importanthelps override conflicting styles. Consider reviewing if the use of!importantis strictly necessary to avoid potential future specificity challenges.app/client/packages/design-system/ads/src/Select/rc-styles.css (5)
7-12: Formatting & Spacing Adjustments
Extra whitespace/newlines have been introduced after the.rc-selectand.rc-select-disabledrules. If these are intentional for readability or to maintain block‐separation consistency, they’re fine—but please verify that the project’s formatting guidelines are met.
79-85: Multi‑Select Container Styling
The multi‑select container now usesflex-wrapwith a1pxpadding and a1px solid #000border (lines 79–85). The black border is a notable design choice—please double‑check if this fits the overall theme.
86-93: Multi‑Select Item Styling
The selection items in the multi‑select variant have an updated background (#bbb), border-radius, and margin/padding (lines 86–93). Confirm that the chosen background color aligns with the design system.
131-137: Multi‑Select Search Input Background Color
The search input now has a background set torgba(255, 0, 0, 0.2)(lines 131–137). This semi‑transparent red may be a debugging remnant or an intentional design choice; please confirm that it aligns with the desired UI theme.
290-299: Virtual List Scrollbar Customization
Custom styling for the virtual list scrollbar is applied with fixed dimensions (5px !important) and a design token for the thumb’s background color (line 299). The use of!importantshould be double-checked to ensure it doesn’t conflict with other styles.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/Select/Select.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Select/Select.tsx(4 hunks)app/client/packages/design-system/ads/src/Select/rc-styles.css(7 hunks)app/client/packages/design-system/ads/src/Select/styles.css(7 hunks)app/client/packages/design-system/ads/src/Tag/Tag.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Tag/Tag.types.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (32)
app/client/packages/design-system/ads/src/Tag/Tag.types.tsx (2)
6-6: Consider addressing the TODO comment.The comment suggests consolidating with the
Kindtype from__config__/types. This could help maintain consistency across the design system.Would you like me to help implement this consolidation?
7-7: LGTM! The addition of "info" kind enhances tag variants.The change aligns with common tag variants and improves the component's flexibility.
app/client/packages/design-system/ads/src/Tag/Tag.styles.tsx (1)
81-83: LGTM! The line-height adjustment improves text alignment.The scoped style ensures consistent text rendering within tags.
app/client/packages/design-system/ads/src/Select/Select.tsx (1)
41-42: LGTM! Clean implementation of search functionality.The search state and ref are properly initialized.
app/client/packages/design-system/ads/src/Select/styles.css (8)
3-6: Clarify Spacing Variables
Explicitly setting--select-padding-xand--select-padding-ywith clear inline comments improves readability and future maintenance.
92-99: Refine Search Input Padding Calculation
The updated calculation for--select-search-input-padding-rightin the clear/arrow group ensures proper space allocation for icons. This creates consistency in the layout.
208-208: Reset Dropdown Container Padding
Settingpadding: 0;for the dropdown container streamlines the layout and removes any unintended spacing. Confirm this change aligns with the overall design specifications.
226-227: Enhance Option Styling with Spacing and Alignment
Addingmargin-inlineandmargin-bottomimproves the spacing between options, and switching todisplay: flex;withalign-items: center;ensures proper vertical alignment.Also applies to: 234-236
238-240: Apply Consistent Top Margin to First Option
The addition of a top margin for the first option item enhances the visual rhythm within the dropdown.
269-271: Indent Grouped Options for Better Hierarchy
Usingmargin-leftfor grouped options visually distinguishes them from ungrouped items, reinforcing a clear hierarchical structure.
315-320: Reset Active Option Background
Resetting the background color for active options (when not hovered) usingunset !importantensures a clean, consistent UI state.
349-355: Streamline Search Input Appearance
The new search input styles remove redundant borders and rounding, providing a more seamless look within the dropdown. Verify that focus or active state cues remain sufficiently visible for accessibility.app/client/packages/design-system/ads/src/Select/rc-styles.css (20)
13-25: Loading Icon Style Added
The new rule for.rc-select-show-arrow.rc-select-loading .rc-select-arrow-icon::afterdefines the loading icon appearance and animation via the keyframercSelectLoadingIcon. This effectively adds rotation and visual feedback during loading. Consider verifying cross-browser compatibility for pseudo-element animations.
31-35: Hiding the Search Input
The.rc-select .rc-select-selection-search-inputrule now usesappearance: none;anddisplay: none;(noted by the change at line 33) to hide the element initially. This is appropriate if the search UI is meant to be conditionally shown.
36-40: Hiding the WebKit Cancel Button
The styles for the::-webkit-search-cancel-buttonensure it is completely hidden by setting bothdisplay: none;andappearance: none;. This is a solid approach to keep the UI clean in WebKit browsers.
41-53: Single Select Layout Updates
The flex container setup for.rc-select-single .rc-select-selectorand the full-width settings for search-related elements (lines 46–53) are well adjusted to ensure responsive behavior in the single select variant.
54-61: Positioning for Selected Items
The absolute positioning for.rc-select-single .rc-select-selector .rc-select-selection-itemand its placeholder (lines 54–61) properly controls overflow and pointer events. This helps ensure that long text is truncated as expected.
69-69: Minor Whitespace Improvement
A small whitespace addition (line 69) appears to improve visual separation in the stylesheet.
70-78: Clean Look for Non‐Customized Input
Removing borders and outlines for.rc-select-single:not(.rc-select-customize-input) .rc-select-selector .rc-select-selection-search-input(lines 70–78) helps achieve a minimalist design.
94-98: Disabled Item Appearance for Multi‑Select
The.rc-select-multiple .rc-select-selector .rc-select-selection-item-disabledrule applies a not‑allowed cursor and reduces opacity to 0.5 (lines 94–98), which is a clear way to denote a disabled state.
99-109: Overflow Handling in Multi‑Select
Styling for.rc-select-multiple .rc-select-selector .rc-select-selection-overflowand its items (lines 99–109) uses flex display and wrapping to handle excess items effectively.
110-114: Multi‑Select Search Container Layout
The rule for.rc-select-multiple .rc-select-selector .rc-select-selection-search(lines 110–114) uses relative positioning and max‑width, ensuring that the search input area in multi‑select is properly contained.
115-120: Padding & Font for Multi‑Select Search Elements
The combined styles for the search input and its mirror (lines 115–120) define padding and set thefont-familyusing a design variable (var(--ads-v2-font-family)). This promotes consistency across components.
138-147: Clearable Multi‑Select Enhancements
The adjustments for.rc-select-allow-clear.rc-select-multiple .rc-select-selector(lines 138–140) and the positioning of the clear icon in.rc-select-allow-clear .rc-select-clear(lines 142–147) improve the usability of the clear functionality.
148-158: Multi‑Select Arrow & Indicator Positioning
The style rules for.rc-select-show-arrow.rc-select-multiple .rc-select-selector(lines 148–150) and the absolute positioning for.rc-select-show-arrow .rc-select-arrow(lines 152–158) ensure that the arrow is correctly aligned in both multi‑select and regular variants.
169-174: Dropdown Container Styling
The dropdown container (.rc-select-dropdown) now specifies a minimum height and a white background (with a change noted at line 174), which helps guarantee a consistent dropdown appearance.
175-178: Dropdown Hidden State
The hidden state for the dropdown (.rc-select-dropdown-hidden) is properly managed by settingdisplay: none;(with the change marked at line 178), ensuring that the component can be toggled without layout shifts.
184-190: Select Item & Group Styling
The font size, line height, padding for.rc-select-item(line 184) as well as the styling for item groups in.rc-select-item-group(line 190) create a clear visual hierarchy and readability for list items.
191-213: Option Elements and Their States
The rules for.rc-select-item-optionand its related classes—including the grouped option (.rc-select-item-option-grouped), the option state indicator, active (.rc-select-item-option-active), and disabled (.rc-select-item-option-disabled)—are defined with appropriate positioning and colors (markers at lines 194, 198, 205, 209, 213). This cohesive styling facilitates clear visual feedback.
214-242: Empty State & Choice Zoom Transitions
The empty state styling (.rc-select-item-empty) combined with the smooth zoom transitions for choice appearance and exit (rules.rc-select-selection__choice-zoomand its variants from lines 219 to 242) deliver a polished interaction. The transitions’ timing (0.3s) and scaling (from 0.5 to 1) are well balanced.
243-289: Dropdown Slide Animations
A series of animations for dropdown transitions have been defined for both upward and downward movements. This includes:
- Slide-up enter/appear (lines 243–252)
- Slide-up leave (lines 253–261)
- Slide-up animations for bottom placements (lines 262–269 and 270–275)
- Slide-down animations for top placements (lines 276–283 and 284–289)
The use of different cubic-bezier timing functions and explicit animation names (e.g.
rcSelectDropdownSlideUpIn,rcSelectDropdownSlideDownOut) ensures smooth and context‑sensitive transitions.
300-364: Keyframes for Transitions & Loading Animation
The keyframe definitions for dropdown transitions (rcSelectDropdownSlideUpIn,rcSelectDropdownSlideUpOut,rcSelectDropdownSlideDownIn, andrcSelectDropdownSlideDownOut) and the loading icon (rcSelectLoadingIcon) are all defined with clear start and end states. The animations correctly handle opacity, transform origins, and scaling to produce smooth transitions.
| // @ts-expect-error type error | ||
| selectedOptions.filter((opt) => opt.value !== unselectedOption.value), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix TypeScript errors instead of using ts-expect-error.
Multiple type errors are being suppressed. Consider fixing them by properly typing the selectedOptions state and option parameters.
-const [selectedOptions, setSelectedOptions] = useState([]);
+interface Option {
+ label: string;
+ value: string;
+ key?: string;
+}
+const [selectedOptions, setSelectedOptions] = useState<Option[]>([]);Also applies to: 1010-1011, 1027-1028
| options: Array.from({ length: 1000 }, (_, i) => ({ | ||
| label: `Option ${i + 1}`, | ||
| value: `value ${i + 1}`, | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pagination or virtualization for large option lists.
Generating 1000 options at once could impact performance. Consider implementing pagination or using virtualization for better performance.
jsartisan
force-pushed
the
chore/multi-select-updates
branch
from
714ee4d to
2d88e90
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
app/client/packages/design-system/ads/src/Select/rc-styles.css (1)
121-129: Invalid CSS Property Value
In the rule for.rc-select-multiple .rc-select-selector .rc-select-selection-search-mirror, the propertyposition: none;(line 125) is not a valid CSS value. It should be removed or replaced with a valid value (e.g., using only the previously declaredposition: absolute;).
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/Select/Select.tsx (2)
56-72: Consider debouncing the search input focus.The setTimeout hack to focus the search input could lead to race conditions. Consider using a more robust solution.
- setTimeout(() => { - if (!searchRef.current) return; - - searchRef.current?.focus(); - }, 200); + // Use requestAnimationFrame for better timing + requestAnimationFrame(() => { + if (!searchRef.current) return; + searchRef.current?.focus(); + });
112-124: Consider memoizing the tagRender function.Since tagRender is recreated on every render, consider memoizing it for better performance.
+const memoizedTagRender = React.useCallback((props) => { + if (rest.tagRender) { + return rest.tagRender(props); + } + + const { closable, label, onClose } = props; + + return ( + <Tag isClosable={closable} kind="info" onClose={onClose}> + {label} + </Tag> + ); +}, [rest.tagRender]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/packages/design-system/ads/src/Select/Select.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Select/Select.tsx(5 hunks)app/client/packages/design-system/ads/src/Select/rc-styles.css(7 hunks)app/client/packages/design-system/ads/src/Select/styles.css(5 hunks)app/client/packages/design-system/ads/src/Tag/Tag.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Tag/Tag.types.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/client/packages/design-system/ads/src/Tag/Tag.types.tsx
- app/client/packages/design-system/ads/src/Tag/Tag.styles.tsx
- app/client/packages/design-system/ads/src/Select/styles.css
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (36)
app/client/packages/design-system/ads/src/Select/Select.tsx (1)
87-102: LGTM! Clean implementation of search input in dropdown.The conditional rendering of SearchInput and proper state management looks good.
app/client/packages/design-system/ads/src/Select/Select.stories.tsx (2)
991-994: Consider pagination or virtualization for large option lists.Generating 1000 options at once could impact performance. Consider implementing pagination or using virtualization for better performance.
999-1001: Fix TypeScript errors instead of using ts-expect-error.Multiple type errors are being suppressed. Consider fixing them by properly typing the selectedOptions state and option parameters.
app/client/packages/design-system/ads/src/Select/rc-styles.css (33)
7-12: Whitespace and Disabled State Styling
Minor whitespace addition is observed at line 7, and the new disabled state styling on.rc-select-disabled(lines 8–11) clearly sets a “not-allowed” cursor for disabled inputs. This improves the UX when interacting with inactive states.
13-24: Loading Icon Animation Styling
The new styling for the loading state on.rc-select-show-arrow.rc-select-loading .rc-select-arrow-icon::afteris well defined—with explicit dimensions, border settings, and an animation (rcSelectLoadingIcon) for a rotating effect. Please verify cross-browser compatibility to ensure a consistent experience.
26-29: Placeholder Styling Adjustment
The placeholder style now uses a reduced opacity and disables pointer events, which improves the visual hierarchy without interfering with interactions.
31-34: Search Input Visibility
Hiding the search input viaopacity: 0(in addition toappearance: none;) is an interesting choice. Please double-check that this method does not inadvertently affect layout or focus management across browsers.
36-39: Search Cancel Button Hiding
The cancel button for the search input is effectively hidden using the::-webkit-search-cancel-buttonpseudo-element. This approach is appropriate for ensuring a clean UI in WebKit-based browsers.
41-60: Single Select Layout Adjustments
The flex layout for the single select variant is clearly defined—with absolute positioning for selection items and placeholders ensuring proper alignment and text overflow handling. The modifications are consistent with the design goals.
61-78: Overflow and Input Customization
Settings for text overflow and the removal of borders/outlines in the customized input (using the:not(.rc-select-customize-input)selector) are neatly implemented to maintain a clean appearance.
79-104: Multiple Select Basic Styling
The multiple select container now uses a flex layout with wrapping, along with explicit padding and border settings (e.g.border: 1px solid #000). These changes help manage the layout of selection items consistently.
109-120: Multiple Select Search and Overflow Layout
The overflow items and search input for the multiple select variant are styled with proper padding and font settings, ensuring that text does not overflow and the component retains its structure.
131-136: Debugging Background on Search Input?
The background for.rc-select-multiple .rc-select-selector .rc-select-selection-search-inputis set torgba(255, 0, 0, 0.2)(lines 134–135), which imparts a red tint. Please confirm whether this styling is intentional for design emphasis or a leftover from debugging.
137-140: Allow Clear Padding Adjustment
The addition of extra right padding on.rc-select-allow-clear.rc-select-multiple .rc-select-selector(line 139) is a sensible adjustment to accommodate the clear icon.
142-146: Clear Button Positioning
The absolute positioning for the clear button in.rc-select-allow-clear .rc-select-clear(lines 142–146) is properly configured to ensure it displays in the correct location relative to the select component.
148-150: Additional Padding for Arrow in Multi-select
Adding right padding for the multi-select variant under the.rc-select-show-arrow.rc-select-multiple .rc-select-selectorclass prevents the arrow from overlapping with the selection items.
152-157: Arrow Icon Positioning
The arrow icon styling in.rc-select-show-arrow .rc-select-arrowuses absolute positioning and disables pointer events to ensure it remains purely decorative while aligning correctly.
159-167: Arrow Icon Indicator Styling
The pseudo-element styling for.rc-select-show-arrow .rc-select-arrow-icon::aftercreates a clear visual indicator using transparent borders and a translation effect. This is effective for indicating dropdown capability.
169-174: Dropdown Container Styling
The dropdown container (styled under.rc-select-dropdown) is given a minimum height and an absolute position with a white background, ensuring that it appears prominently when activated.
175-178: Dropdown Hidden State
The utility class.rc-select-dropdown-hiddencorrectly appliesdisplay: none;to hide the dropdown when necessary.
179-217: Dropdown Item and Group Styling
The styles for dropdown items (.rc-select-item), groups (.rc-select-item-group), options, and grouped options are consistently defined. Font sizes, padding, and colors are adjusted to enhance readability and visual hierarchy.
219-221: Choice Zoom Transition
The transition on.rc-select-selection__choice-zoom(line 220) provides smooth scaling for selection animations, enhancing interactivity.
223-241: Choice Zoom Animation Effects
The appear and leave animations for choices (lines 223–241) combine opacity and scaling to create an engaging visual effect. This implementation meets modern UI animation standards.
243-251: Dropdown Slide-Up Enter Animation
The slide-up enter animation, defined for dropdown transitions (lines 243–251), employs a 0.3s duration with a cubic-bezier timing function. The use ofanimation-play-state: paused;which is then switched to running ensures controlled entry transitions.
252-260: Dropdown Slide-Up Leave Animation
Similar care is taken with the slide-up leave animation (lines 252–260), which uses a complementary easing curve to ensure the dropdown exits gracefully.
262-268: Slide-Up Enter Active for Bottom Placement
For dropdowns placed at the bottom, the active enter animation (lines 262–268) leveragesrcSelectDropdownSlideUpInwith the animation set to run, ensuring a smooth reveal effect.
269-274: Slide-Up Leave Active for Bottom Placement
The corresponding leave animation for bottom placements (lines 269–274) employsrcSelectDropdownSlideUpOutto harmonize with the entry animation, providing visual consistency.
276-282: Slide-Down Enter Animation for Top Placement
Dropdowns positioned on top now animate usingrcSelectDropdownSlideDownIn(lines 276–282), which appropriately reverses the direction for a natural appearance from the top.
283-288: Slide-Down Leave Animation for Top Placement
The slide-down leave animation (lines 283–288), defined withrcSelectDropdownSlideDownOut, ensures that top-positioned dropdowns exit in a way that mirrors their entry, contributing to a unified interaction design.
290-293: Custom Scrollbar Styling
The scrollbar container under.rc-virtual-list-scrollbaris set to a fixed width and height (with!importantto enforce consistency). This custom styling should be tested across different environments to ensure responsiveness.
295-298: Scrollbar Thumb Styling
The thumb element of the custom scrollbar is styled with a background color drawn from a CSS variable and a high border-radius for a modern look. Please verify that the chosen color meets accessibility contrast standards.
300-312: Keyframes for Slide-Up In Animation
The@keyframes rcSelectDropdownSlideUpInblock (lines 300–312) smoothly transitions the dropdown from a scaled-down, transparent state to full visibility. The use of a scaleY transform with origin anchoring is effective.
314-326: Keyframes for Slide-Up Out Animation
The inverse@keyframes rcSelectDropdownSlideUpOut(lines 314–326) mirrors the “in” animation appropriately, ensuring a consistent hide effect with smooth scaling and opacity reduction.
328-340: Keyframes for Slide-Down In Animation
The@keyframes rcSelectDropdownSlideDownIn(lines 328–340) logically transitions the dropdown for top placements, with a shift in the transform origin to create a natural slide-in effect.
342-354: Keyframes for Slide-Down Out Animation
Similarly, the@keyframes rcSelectDropdownSlideDownOut(lines 342–354) ensures that the dropdown exits smoothly with a scaleY transition that mirrors the entry animation.
356-365: Keyframes for Loading Icon Animation
The rotation animation defined by@keyframes rcSelectLoadingIcon(lines 356–365) is straightforward and effective for simulating a loading spinner. The timing of 0.5s for a full rotation should feel responsive without being distracting.
| <Checkbox | ||
| // making it read only as it is interfering with selection of options of rc-select | ||
| isReadOnly | ||
| isSelected={Boolean( | ||
| selectedOptions.find( | ||
| (selectedOption) => selectedOption.value == option.value, | ||
| ), | ||
| )} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using controlled checkbox component.
The isReadOnly prop is used as a workaround. Consider implementing a proper controlled checkbox component.
-<Checkbox
- isReadOnly
- isSelected={Boolean(
- selectedOptions.find(
- (selectedOption) => selectedOption.value == option.value,
- ),
- )}
->
+<Checkbox
+ isSelected={Boolean(
+ selectedOptions.find(
+ (selectedOption) => selectedOption.value === option.value,
+ ),
+ )}
+ onChange={(e) => {
+ const { checked } = e.target;
+ if (checked) {
+ setSelectedOptions([...selectedOptions, option]);
+ } else {
+ setSelectedOptions(
+ selectedOptions.filter((opt) => opt.value !== option.value)
+ );
+ }
+ }}
+>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Checkbox | |
| // making it read only as it is interfering with selection of options of rc-select | |
| isReadOnly | |
| isSelected={Boolean( | |
| selectedOptions.find( | |
| (selectedOption) => selectedOption.value == option.value, | |
| ), | |
| )} | |
| > | |
| <Checkbox | |
| isSelected={Boolean( | |
| selectedOptions.find( | |
| (selectedOption) => selectedOption.value === option.value, | |
| ), | |
| )} | |
| onChange={(e) => { | |
| const { checked } = e.target; | |
| if (checked) { | |
| setSelectedOptions([...selectedOptions, option]); | |
| } else { | |
| setSelectedOptions( | |
| selectedOptions.filter((opt) => opt.value !== option.value) | |
| ); | |
| } | |
| }} | |
| > |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Select/Select.tsx (1)
56-72: Consider using requestAnimationFrame instead of setTimeout.The setTimeout hack for focusing the search input could be more reliable using requestAnimationFrame.
- setTimeout(() => { + requestAnimationFrame(() => { if (!searchRef.current) return; searchRef.current?.focus(); - }, 200); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/package.json(2 hunks)app/client/packages/design-system/ads/src/Select/Select.tsx(5 hunks)app/client/src/components/formControls/DropDownControl.test.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/components/formControls/DropDownControl.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-lint / client-lint
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (4)
app/client/packages/design-system/ads/src/Select/Select.tsx (3)
41-42: LGTM: Clean state management implementation.The search functionality is properly implemented using React hooks.
87-102: LGTM: Clean dropdown render implementation.The search input integration in the dropdown is well implemented.
112-124: LGTM: Proper tag rendering with fallback.The tag rendering logic properly handles custom tag renderers while providing a default implementation.
app/client/package.json (1)
32-32: LGTM: Improved test output visibility.Removing the --silent flag will help with debugging test failures by providing more detailed output.
| value: "value 11", | ||
| }, | ||
| { | ||
| label: "Option 2", |
There was a problem hiding this comment.
Please check how the component works with overflow. So if you add a long label, then the item will be displayed in several lines. If you add a long value, only the +1 badge will be displayed without the ability to delete it.
There was a problem hiding this comment.
So if you add a long label, then the item will be displayed in several lines.
I think This should not be done as the options can be emails or book names or anything needs full name for better context. It becomes confusing with the ellipsis in place. But I have implemented it.
If you add a long value, only the +1 badge will be displayed without the ability to delete it.
I am afraid, there is no proper way to make it like that. The tag behavior is set as responsive. It's just how the version of rc-select we are using is built. I have solved for when only one is selected, but the issue can still happen for 2 selected values.
There was a problem hiding this comment.
IMO, best UX is to not use tags at all and show like "2 values selected" when more than 1 value is selected.
cc: @ichik
There was a problem hiding this comment.
I think This should not be done as the options can be emails or book names or anything needs full name for better context. It becomes confusing with the ellipsis in place. But I have implemented it.
I think this is a common behavior. Usually, a title is added to see the entire value.
I am afraid, there is no proper way to make it like that. The tag behavior is set as responsive. It's just how the version of rc-select we are using is built. I have solved for when only one is selected, but the issue can still happen for 2 selected values.
Can we set the max-width for the first tag if we have +1 tag so that overflow does not occur. I mean, something like this: very long long value ... +1
There was a problem hiding this comment.
Using chips has and advantage of being more easily deselectable than just showing text about the number of options. Growing the box vertically could be an option: https://eui.elastic.co/#/forms/combo-box
There was a problem hiding this comment.
Can we set the max-width for the first tag if we have +1 tag so that overflow does not occur. I mean, something like this: very long long value ... +1
I was able to do it. Can you try?
| label={option.label} | ||
| value={option.value} | ||
| > | ||
| <Checkbox |
There was a problem hiding this comment.
I think it should all be encapsulated in a component to make this component easier to use. Anyway, it's not for this PR.
There was a problem hiding this comment.
the only way to do is make APi like
<Select options={...} />instead of
<Select>
<Option>Option1</Option>
<Option>Option1</Option>
</Select>There was a problem hiding this comment.
This can be a solution, yes. We can also use specialized options with checkboxes.
Yes, you are right. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/client/packages/design-system/ads/src/Checkbox/Checkbox.tsx(1 hunks)app/client/packages/design-system/ads/src/Select/Select.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Select/Select.tsx(5 hunks)app/client/packages/design-system/ads/src/Select/styles.css(7 hunks)app/client/packages/design-system/ads/src/Tag/Tag.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/client/packages/design-system/ads/src/Checkbox/Checkbox.tsx
- app/client/packages/design-system/ads/src/Tag/Tag.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Select/styles.css
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (4)
app/client/packages/design-system/ads/src/Select/Select.tsx (1)
33-37: LGTM! Smart handling of maxTagCount.The dynamic maxTagCount logic elegantly handles both single and multiple selections, using responsive mode for multiple selections.
app/client/packages/design-system/ads/src/Select/Select.stories.tsx (3)
991-994: Consider pagination or virtualization for large option lists.Generating 1000 options at once could impact performance.
1026-1034: Consider using controlled checkbox component.The isReadOnly prop is used as a workaround.
984-986: Verify handling of long labels in the UI.The long label might cause layout issues in the dropdown. Please ensure that the UI handles text overflow gracefully.
Run the following script to check for other instances of potentially problematic long labels:
| const handleDropdownVisibleChange = (open: boolean) => { | ||
| if (open) { | ||
| // this is a hack to get the search input to focus when the dropdown is opened | ||
| // the reason is, rc-select does not support putting the search input in the dropdown | ||
| // and rc-select focus its native searchinput element on dropdown open, but we need to focus the search input | ||
| // so we use a timeout to focus the search input after the dropdown is opened | ||
| setTimeout(() => { | ||
| if (!searchRef.current) return; | ||
|
|
||
| searchRef.current?.focus(); | ||
| }, 200); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| setSearchValue(""); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up timeout on component unmount.
The timeout used for focusing the search input should be cleaned up to prevent memory leaks.
Apply this diff to add cleanup:
+const timeoutRef = useRef<NodeJS.Timeout>();
const handleDropdownVisibleChange = (open: boolean) => {
if (open) {
- setTimeout(() => {
+ timeoutRef.current = setTimeout(() => {
if (!searchRef.current) return;
searchRef.current?.focus();
}, 200);
return;
}
setSearchValue("");
};
+useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleDropdownVisibleChange = (open: boolean) => { | |
| if (open) { | |
| // this is a hack to get the search input to focus when the dropdown is opened | |
| // the reason is, rc-select does not support putting the search input in the dropdown | |
| // and rc-select focus its native searchinput element on dropdown open, but we need to focus the search input | |
| // so we use a timeout to focus the search input after the dropdown is opened | |
| setTimeout(() => { | |
| if (!searchRef.current) return; | |
| searchRef.current?.focus(); | |
| }, 200); | |
| return; | |
| } | |
| setSearchValue(""); | |
| }; | |
| const timeoutRef = useRef<NodeJS.Timeout>(); | |
| const handleDropdownVisibleChange = (open: boolean) => { | |
| if (open) { | |
| // this is a hack to get the search input to focus when the dropdown is opened | |
| // the reason is, rc-select does not support putting the search input in the dropdown | |
| // and rc-select focus its native searchinput element on dropdown open, but we need to focus the search input | |
| // so we use a timeout to focus the search input after the dropdown is opened | |
| timeoutRef.current = setTimeout(() => { | |
| if (!searchRef.current) return; | |
| searchRef.current?.focus(); | |
| }, 200); | |
| return; | |
| } | |
| setSearchValue(""); | |
| }; | |
| useEffect(() => { | |
| return () => { | |
| if (timeoutRef.current) { | |
| clearTimeout(timeoutRef.current); | |
| } | |
| }; | |
| }, []); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/client/packages/design-system/ads/src/Select/Select.mdx (1)
54-57: New Grouping Section for Checkbox OptionsThe addition of the "Grouping with Checkbox in options" section is a clear enhancement that aligns with the PR objectives. This new Canvas element demonstrates how grouping with checkboxes can be integrated into the Select component. Please double-check that the referenced story,
SelectStories.SelectWithCheckboxAndGroup, is correctly implemented in the stories file and consider adding a short explanatory note to guide users on the purpose and behavior of this feature.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/design-system/ads/src/Select/Select.mdx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
Don't know what is happening, but the ads button that we use in tags is conflicting with select behaviour :/ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/Select/Select.tsx (2)
33-37: Simplify maxTagCount logic.The nested ternary operation makes the code harder to read.
Consider using a more readable approach:
-maxTagCount = isMultiSelect - ? props.value?.length > 1 - ? "responsive" - : 1 - : undefined, +maxTagCount = !isMultiSelect ? undefined : props.value?.length > 1 ? "responsive" : 1,
92-107: Consider extracting dropdown render logic to a separate component.The dropdown render logic could be moved to a separate component to improve maintainability and reusability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/packages/design-system/ads/src/Select/Select.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Select/Select.tsx(5 hunks)app/client/packages/design-system/ads/src/Select/styles.css(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/packages/design-system/ads/src/Select/styles.css
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (3)
app/client/packages/design-system/ads/src/Select/Select.tsx (1)
61-77: Clean up timeout on component unmount.The timeout used for focusing the search input should be cleaned up to prevent memory leaks.
app/client/packages/design-system/ads/src/Select/Select.stories.tsx (2)
991-994: Consider pagination or virtualization for large option lists.Generating 1000 options at once could impact performance.
1027-1035: Consider using controlled checkbox component.The isReadOnly prop is used as a workaround. Consider implementing a proper controlled checkbox component.
I was able fix the above bug as well.. had to go into their source code to see what's happening. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/13132807117. |
|
Deploy-Preview-URL: https://ce-38954.dp.appsmith.com |
jsartisan
added
ok-to-test
jsartisan
force-pushed
the
chore/multi-select-updates
branch
from
f57d41b to
d12f864
Compare
jsartisan
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/client/packages/design-system/ads/src/Select/Select.stories.tsx (2)
991-994: Consider pagination or virtualization for large option lists.Generating 1000 options at once could impact performance.
Consider implementing:
- Pagination to load options in chunks
- Virtual scrolling (already enabled via
virtualprop)- Lazy loading of options
1028-1036: Consider using controlled checkbox component.The isReadOnly prop is used as a workaround. Consider implementing a proper controlled checkbox component.
Apply this diff:
-<Checkbox - isReadOnly - isSelected={Boolean( - selectedOptions.find( - (selectedOption) => selectedOption.value == option.value, - ), - )} -> +<Checkbox + isSelected={Boolean( + selectedOptions.find( + (selectedOption) => selectedOption.value === option.value, + ), + )} + onChange={(e) => { + const { checked } = e.target; + if (checked) { + setSelectedOptions([...selectedOptions, option]); + } else { + setSelectedOptions( + selectedOptions.filter((opt) => opt.value !== option.value) + ); + } + }} +>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/client/cypress/support/Pages/GitSync.ts(1 hunks)app/client/package.json(1 hunks)app/client/packages/design-system/ads/src/Checkbox/Checkbox.tsx(1 hunks)app/client/packages/design-system/ads/src/Select/Select.mdx(1 hunks)app/client/packages/design-system/ads/src/Select/Select.stories.tsx(2 hunks)app/client/packages/design-system/ads/src/Select/Select.tsx(5 hunks)app/client/packages/design-system/ads/src/Select/rc-styles.css(7 hunks)app/client/packages/design-system/ads/src/Select/styles.css(8 hunks)app/client/packages/design-system/ads/src/Tag/Tag.styles.tsx(1 hunks)app/client/packages/design-system/ads/src/Tag/Tag.tsx(2 hunks)app/client/packages/design-system/ads/src/Tag/Tag.types.tsx(1 hunks)app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/TableOrSpreadsheetDropdown/index.tsx(0 hunks)app/client/src/components/formControls/DropDownControl.test.tsx(1 hunks)app/client/src/components/formControls/DropDownControl.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- app/client/src/components/editorComponents/WidgetQueryGeneratorForm/CommonControls/TableOrSpreadsheetDropdown/index.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- app/client/packages/design-system/ads/src/Checkbox/Checkbox.tsx
- app/client/packages/design-system/ads/src/Select/Select.mdx
- app/client/src/components/formControls/DropDownControl.test.tsx
- app/client/package.json
- app/client/packages/design-system/ads/src/Tag/Tag.types.tsx
- app/client/packages/design-system/ads/src/Tag/Tag.styles.tsx
- app/client/packages/design-system/ads/src/Tag/Tag.tsx
- app/client/src/components/formControls/DropDownControl.tsx
- app/client/packages/design-system/ads/src/Select/styles.css
- app/client/packages/design-system/ads/src/Select/rc-styles.css
🧰 Additional context used
📓 Path-based instructions (1)
app/client/cypress/support/Pages/GitSync.ts (1)
Pattern app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
🔇 Additional comments (4)
app/client/packages/design-system/ads/src/Select/Select.tsx (2)
92-107: LGTM!Clean implementation of the dropdown render with conditional search input.
61-77:⚠️ Potential issueAdd cleanup for setTimeout to prevent memory leaks.
The timeout used for focusing the search input should be cleaned up when the component unmounts.
Apply this diff:
+const timeoutRef = useRef<NodeJS.Timeout>(); const handleDropdownVisibleChange = (open: boolean) => { if (open) { - setTimeout(() => { + timeoutRef.current = setTimeout(() => { if (!searchRef.current) return; searchRef.current?.focus(); }, 200); return; } setSearchValue(""); }; +useEffect(() => { + return () => { + if (timeoutRef.current) { + clearTimeout(timeoutRef.current); + } + }; +}, []);Likely invalid or redundant comment.
app/client/cypress/support/Pages/GitSync.ts (1)
74-75: LGTM!The selector update aligns with the new Select component implementation.
app/client/packages/design-system/ads/src/Select/Select.stories.tsx (1)
999-1001: 🛠️ Refactor suggestionFix TypeScript errors instead of using ts-expect-error.
Multiple type errors are being suppressed. Consider fixing them by properly typing the selectedOptions state.
Apply this diff:
-const [selectedOptions, setSelectedOptions] = useState<DefaultOptionType[]>([]); +interface Option { + label: string; + value: string; + key?: string; +} +const [selectedOptions, setSelectedOptions] = useState<Option[]>([]);Likely invalid or redundant comment.
jsartisan
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/client/packages/design-system/ads/src/Select/styles.css (3)
340-342: Eliminate Duplicate Hover PropertyThere is a redundant declaration of
--select-option-color-bgin the hover state. Removing the duplicate line would simplify the CSS without affecting the intended hover behavior.
376-384: Styled Search Input in DropdownThe new styling for the search input—removing three sides of the border and applying a bottom border with emphasis—creates a minimalist look. However, using
outline: none !important;removes the focus indicator, which could impact accessibility. It may be beneficial to introduce an alternative focus style to maintain accessibility standards.
411-414: Potential Duplication in Virtual List StylesThere appears to be a duplicate rule setting the padding for
.rc-virtual-list(also defined in lines 243-246). Consider consolidating these to avoid redundancy and ease future maintenance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/packages/design-system/ads/src/Select/styles.css(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: storybook-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
🔇 Additional comments (14)
app/client/packages/design-system/ads/src/Select/styles.css (14)
21-23: Cursor Style Update in Search ModeUpdating the cursor to
unsetfor.ads-v2-select.rc-select-show-search *should prevent unwanted pointer styling during search input. Ensure this change aligns with the expected user interaction across browsers.
190-193: Updated "+9 Item" StylingThe new background, border, text color, and line-height for the overflow indicator ensure it stands out per the refreshed design tokens. Confirm that this visual treatment integrates well with the overall theme.
214-214: Dropdown Padding AdjustmentSetting
padding: 0;on the dropdown reduces extra spacing and achieves a flush edge layout. Please verify that this change does not introduce unintended layout shifts in different dropdown contexts.
230-241: Select Option Layout EnhancementIntroducing
margin-inline: var(--ads-v2-spaces-2), along withdisplay: flexandalign-items: center, improves the alignment of option items. The updated background color aligns with the new visual design. It would be good to test various option content lengths to ensure consistency.
243-246: Virtual List Padding for DropdownApplying a consistent vertical padding (
padding-topandpadding-bottomofvar(--ads-v2-spaces-2)) to.rc-virtual-listenhances the spacing within the dropdown. This aligns well with the overall spacing guidelines.
248-253: Conditional Padding Removal for Grouped ItemsUsing the
:has()and:is()pseudo-classes to remove top padding when the first dropdown item is a group helps maintain consistent spacing. Please verify browser support for these newer selectors to ensure cross-browser compatibility.
274-291: Grouped Options Margin AdjustmentThe addition of
margin-left: var(--ads-v2-spaces-2)for grouped options aids in visually separating them from the container edge, refining the grouped layout. This adjustment appears to be in line with the design specifications.
320-321: Full-Width Option ContentApplying
width: 100%;ensures that the option content fully occupies the available space, which can help maintain consistent alignment within the dropdown.
323-331: Truncated Option LabelsReformatting the label span with
display: -webkit-boxand-webkit-line-clampguarantees long labels are neatly truncated. While this approach is effective in WebKit-based browsers, double-check its behavior in non-WebKit environments.
343-347: Selected Option Styling ConsistencyThe style for selected options is clearly defined and leverages the appropriate design tokens. The formatting updates here are cosmetic and maintain consistency.
386-389: Container Query Introduction for SelectAdding
container-type: inline-size;on.ads-v2-select .rc-select-selectoris an innovative step toward using container queries for responsive tag widths. Since this is an emerging feature, please verify its support in the target browsers or consider providing fallback styles.
391-397: Optimized Tag Width CalculationThe use of
calc(60cqw - 16px)to constrain the maximum width of the first tag is creative. However, thecqwunit is relatively new and may not be fully supported across all browsers. Ensure you test this in the environments you target.
399-405: Consistent Tag TruncationThe truncation rules for tag text—using
-webkit-boxand-webkit-line-clamp—maintain layout consistency by preventing overflow. This aligns with similar truncation styles elsewhere in the codebase.
407-409: Hiding Overflow SuffixExplicitly hiding the overflow item suffix cleans up the tag display and supports the intended minimal design. This change should help reduce visual clutter.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/Phone_input_spec.js (1)
27-27: Remove cy.wait() commands.According to the coding guidelines, avoid using cy.wait(). Instead:
- Use cy.intercept() to wait for network requests
- Use cy.should() with retries for UI elements
Example refactor:
-cy.wait(500); +cy.get(widgetInput).should('be.visible');Also applies to: 36-36, 49-49, 61-61, 70-70, 86-86, 88-88, 94-94
🧹 Nitpick comments (2)
app/client/src/pages/Editor/ThemePropertyPane/controls/ThemeFontControl.tsx (1)
46-46: LGTM! Consider adding type safety for options.The addition of the label prop improves accessibility and aligns with select component updates.
Consider adding type safety by creating an interface for the options:
interface FontOption { label: string; value: string; }Then update the props:
- options: string[]; + options: FontOption[];app/client/packages/design-system/ads/src/Select/Select.tsx (1)
33-37: Simplify maxTagCount logic.The nested ternary operator makes the code harder to read. Consider extracting this logic into a separate function or using if-else statements.
-maxTagCount = isMultiSelect - ? props.value?.length > 1 - ? "responsive" - : 1 - : undefined, +maxTagCount = getMaxTagCount(isMultiSelect, props.value), +function getMaxTagCount(isMultiSelect: boolean, value: any[]) { + if (!isMultiSelect) return undefined; + return value?.length > 1 ? "responsive" : 1; +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/client/cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInput_spec.js(10 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/Phone_input_spec.js(5 hunks)app/client/cypress/support/widgetCommands.js(1 hunks)app/client/packages/design-system/ads/src/Select/Select.tsx(5 hunks)app/client/src/components/formControls/DropDownControl.tsx(3 hunks)app/client/src/pages/Editor/ThemePropertyPane/controls/ThemeFontControl.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/src/components/formControls/DropDownControl.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInput_spec.jsapp/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/Phone_input_spec.jsapp/client/cypress/support/widgetCommands.js
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: client-lint / client-lint
- GitHub Check: client-prettier / prettier-check
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (1)
app/client/packages/design-system/ads/src/Select/Select.tsx (1)
60-76: Clean up timeout on component unmount.The timeout used for focusing the search input should be cleaned up to prevent memory leaks.
| cy.searchSelectDropdown("usd"); | ||
|
|
||
| cy.selectDropdownValue("USD - US Dollar"); | ||
|
|
||
| function enterAndTest(text, expected) { | ||
| cy.get(widgetInput).clear(); | ||
| cy.wait(300); |
There was a problem hiding this comment.
Use data- attributes for selectors.*
According to the coding guidelines, use data-* attributes instead of class selectors.
Example refactor:
-cy.get(widgetInput).clear();
+cy.get('[data-cy="currency-input"]').clear();Also applies to: 39-39, 40-40, 41-41, 42-42, 43-43, 44-44
| enterAndTest("100.22", "100.22:100.22:true:string:number:GB:GBP"); | ||
| cy.get(".t--input-currency-change").should("contain", "£"); | ||
| }); | ||
|
|
||
| it("3. should accept 0 decimal option", () => { | ||
| cy.openPropertyPane(widgetName); | ||
| cy.selectDropdownValue(".t--property-control-decimalsallowed input", "0"); | ||
| cy.openSelectDropdown(".t--property-control-decimalsallowed"); | ||
| cy.selectDropdownValue("0"); | ||
| cy.closePropertyPane(); | ||
| cy.wait(500); |
There was a problem hiding this comment.
Remove cy.wait() commands and use proper assertions.
Replace cy.wait() with proper assertions that wait for elements to be in the expected state.
Example refactor:
-cy.wait(300);
+cy.get(widgetInput).should('not.be.disabled');Also applies to: 121-121, 191-191, 192-192, 193-193, 194-194, 195-195, 196-196, 197-197, 198-198
| Cypress.Commands.add("openSelectDropdown", (element) => { | ||
| cy.get(`${element} .rc-select-selection-search-input`).last().scrollIntoView().click({ force: true }); | ||
| }); | ||
|
|
||
| Cypress.Commands.add("selectDropdownValue", (value) => { | ||
| cy.get(".t--dropdown-option") | ||
| .children() | ||
| .contains(value) | ||
| .click({ force: true }); | ||
| }); | ||
|
|
||
| Cypress.Commands.add("searchSelectDropdown", (value) => { | ||
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").click(); | ||
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").should("have.focus"); | ||
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").type(value); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Minimize use of force: true in Cypress commands.
Using force: true bypasses Cypress's built-in retry-ability and can make tests flaky. Consider:
- Waiting for elements to be actionable
- Using proper selectors
- Adding proper assertions
Example refactor:
-cy.get(`${element} .rc-select-selection-search-input`).last().scrollIntoView().click({ force: true });
+cy.get(`${element} .rc-select-selection-search-input`)
+ .last()
+ .scrollIntoView()
+ .should('be.visible')
+ .should('be.enabled')
+ .click();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Cypress.Commands.add("openSelectDropdown", (element) => { | |
| cy.get(`${element} .rc-select-selection-search-input`).last().scrollIntoView().click({ force: true }); | |
| }); | |
| Cypress.Commands.add("selectDropdownValue", (value) => { | |
| cy.get(".t--dropdown-option") | |
| .children() | |
| .contains(value) | |
| .click({ force: true }); | |
| }); | |
| Cypress.Commands.add("searchSelectDropdown", (value) => { | |
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").click(); | |
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").should("have.focus"); | |
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").type(value); | |
| }); | |
| Cypress.Commands.add("openSelectDropdown", (element) => { | |
| cy.get(`${element} .rc-select-selection-search-input`) | |
| .last() | |
| .scrollIntoView() | |
| .should('be.visible') | |
| .should('be.enabled') | |
| .click(); | |
| }); | |
| Cypress.Commands.add("selectDropdownValue", (value) => { | |
| cy.get(".t--dropdown-option") | |
| .children() | |
| .contains(value) | |
| .click({ force: true }); | |
| }); | |
| Cypress.Commands.add("searchSelectDropdown", (value) => { | |
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").click(); | |
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").should("have.focus"); | |
| cy.get(".ads-v2-select__dropdown .ads-v2-input__input-section-input").type(value); | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (4)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_spec.js (2)
62-62: Remove explicit wait.Replace
cy.wait(1000)with proper Cypress commands that wait for elements to be in the expected state.-cy.wait(1000); +cy.get(".t--property-pane-title").should("be.visible").contains("Favorite");
74-77: Use data- attributes for selectors.*Replace class-based selector with a data-* attribute for better test stability.
-cy.get(".t--property-control-placement .rc-select-selection-item").should( +cy.get("[data-cy='placement-dropdown-value']").should( "have.text", "Center", );app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/Phone_input_spec.js (1)
27-28: Replace explicit waits with proper assertions.Multiple
cy.wait()calls with hardcoded times make tests flaky. Replace them with proper Cypress assertions.-cy.wait(500); +cy.get(`.t--widget-${widgetName} input`).should('be.enabled'); -cy.wait(500); +cy.get(".t--widget-textwidget").should('be.visible');Also applies to: 36-37, 45-46, 48-48
app/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_spec.js (1)
25-27: Replace explicit waits with proper assertions.Multiple
cy.wait()calls with hardcoded times make tests flaky. Replace them with proper Cypress assertions.-cy.wait(300); +cy.get(widgetInput).should('be.enabled'); -cy.wait(300); +cy.get(widgetInput).should('have.value', '');Also applies to: 300-300
🧹 Nitpick comments (2)
app/client/src/components/propertyControls/DropDownControl.tsx (2)
125-129: Consider improving class name generation.The class name generation logic could be simplified and made more robust:
- The current implementation may generate invalid class names if the label contains special characters.
- Multiple spaces in the label will result in unnecessary joins.
Consider this improved implementation:
-dropdownClassName={ - this.props.label - ? `t--property-control-${this.props.label.split(" ").join("").toLowerCase()}__select-dropdown` - : "" -} +dropdownClassName={ + this.props.label + ? `t--property-control-${this.props.label.toLowerCase().replace(/[^a-z0-9]+/g, "")}__select-dropdown` + : "" +}
285-301: Add TypeScript improvements to interface definition.The interface could benefit from:
- Better type definitions instead of
any[]- JSDoc documentation for properties
- Addressing the TODO comments about eslint issues
Consider adding proper types and documentation:
export interface DropDownControlProps extends ControlProps { /** Array of options or function returning options */ options?: DropdownOption[] | ((props: ControlProps["widgetProperties"]) => DropdownOption[]); /** Default selected value */ defaultValue?: string; /** Enable virtual scrolling */ virtual?: boolean; /** Placeholder text when no option is selected */ placeholderText: string; /** Placeholder text for search input */ searchPlaceholderText: string; /** Allow multiple selections */ isMultiSelect?: boolean; /** Height of dropdown */ dropdownHeight?: string; /** Enable search functionality */ enableSearch?: boolean; /** Currently selected value */ propertyValue: string; /** Width of dropdown options */ optionWidth?: string; /** Hide sub text in options */ hideSubText?: boolean; /** Use property value directly */ dropdownUsePropertyValue?: boolean; /** Always show selected value */ alwaysShowSelected?: boolean; } interface DropdownOption { value: string; label: string; subText?: string; icon?: string; leftElement?: React.ReactNode; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_spec.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInput_spec.js(10 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_inside_List_spec.js(3 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_spec.js(9 hunks)app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/Phone_input_spec.js(5 hunks)app/client/cypress/locators/ViewWidgets.json(2 hunks)app/client/cypress/locators/Widgets.json(3 hunks)app/client/cypress/locators/commonlocators.json(3 hunks)app/client/cypress/support/widgetCommands.js(1 hunks)app/client/src/components/propertyControls/DropDownControl.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/CurrencyInput/CurrencyInput_spec.js
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_inside_List_spec.jsapp/client/cypress/e2e/Regression/ClientSide/Widgets/Button/ButtonGroup_spec.jsapp/client/cypress/locators/ViewWidgets.jsonapp/client/cypress/e2e/Regression/ClientSide/Widgets/Input/Inputv2_spec.jsapp/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/Phone_input_spec.jsapp/client/cypress/locators/Widgets.jsonapp/client/cypress/locators/commonlocators.jsonapp/client/cypress/support/widgetCommands.js
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
- GitHub Check: chromatic-deployment
- GitHub Check: chromatic-deployment
- GitHub Check: storybook-tests
🔇 Additional comments (5)
app/client/cypress/support/widgetCommands.js (2)
106-110: LGTM! Well-structured search implementation.The search implementation follows best practices with proper focus verification.
89-95: 🛠️ Refactor suggestionImprove reliability of dropdown opening.
The use of
force: truecan make tests flaky. Consider waiting for element to be actionable instead.- cy.get(`${element} .rc-select-selection-search-input`).last().scrollIntoView().click({ force: true }); + cy.get(`${element} .rc-select-selection-search-input`) + .last() + .scrollIntoView() + .should('be.visible') + .should('be.enabled') + .click();Likely invalid or redundant comment.
app/client/cypress/locators/ViewWidgets.json (1)
4-4: LGTM! Simplified selector strategy.The removal of input suffix from chartType selector aligns with the broader selector strategy changes.
app/client/cypress/locators/Widgets.json (1)
8-8: LGTM! Consistent selector updates.The changes maintain consistency with the broader selector strategy updates and properly reinstate previously removed properties.
Also applies to: 34-34, 235-237
app/client/cypress/locators/commonlocators.json (1)
130-130: LGTM! Simplified selector patterns.The removal of input suffix from selectors maintains consistency across the codebase.
Also applies to: 132-132, 134-134, 210-210
|
|
||
| cy.get(".t--property-control-required label") | ||
| .last() | ||
| .click({ force: true }); | ||
|
|
||
| cy.selectDropdownValue(".t--property-control-datatype input", "Number"); | ||
| cy.selectDropdownValue(".t--property-control-datatype", "Number"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate dropdown selection.
The dropdown value is already selected in line 55. This duplicate selection is unnecessary.
-cy.selectDropdownValue(".t--property-control-datatype", "Number");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cy.selectDropdownValue(".t--property-control-datatype", "Number"); | |
| // (The duplicate dropdown selection has been removed) |
| @@ -86,14 +86,29 @@ Cypress.Commands.add("selectDateFormat", (value) => { | |||
| .click({ force: true }); | |||
| }); | |||
|
|
|||
| Cypress.Commands.add("openSelectDropdown", (element) => { | |||
| const isDropdownAlreadyOpen = document.querySelector(`${element} .rc-select-open`); | |||
There was a problem hiding this comment.
Fix potential race condition in DOM query.
Direct DOM query in Cypress command can lead to race conditions. Use Cypress commands for consistency.
- const isDropdownAlreadyOpen = document.querySelector(`${element} .rc-select-open`);
+ let isDropdownAlreadyOpen = false;
+ cy.get(`${element} .rc-select-open`).then($el => {
+ isDropdownAlreadyOpen = $el.length > 0;
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isDropdownAlreadyOpen = document.querySelector(`${element} .rc-select-open`); | |
| let isDropdownAlreadyOpen = false; | |
| cy.get(`${element} .rc-select-open`).then($el => { | |
| isDropdownAlreadyOpen = $el.length > 0; | |
| }); |
Few known bugs:
CleanShot.2025-02-03.at.13.54.24.mp4
/ok-to-test tags="@tag.All"
Summary by CodeRabbit
Caution
🔴 🔴 🔴 Some tests have failed.
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/13163084684
Commit: aa8b7ac
Cypress dashboard.
Tags: @tag.All
Spec:
The following are new failures, please fix them before merging the PR:
- cypress/e2e/Regression/ClientSide/JSObject/JSObject_ForkApp_spec.ts
- cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePickerV2_CSV_spec.js
List of identified flaky tests.Wed, 05 Feb 2025 18:45:55 UTC