feat: introduce new select field based on shadcn implementation with custom interfaces #134
Conversation
📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files within the design system. Key changes include updates to CSS variables, restructuring of component stories to source values from a new module, and significant alterations to the Changes
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
packages/design-system/src/tailwind.css (1)
26-26: Consider updating the dark theme radius for consistency.While the light theme radius has been updated, the dark theme section doesn't have a
--radiusvariable defined. For consistency, you might want to consider adding the--radiusvariable to the dark theme as well.If you decide to add it, you can use the following diff:
.dark { --background: 222.2 84% 4.9%; --foreground: 210 40% 98%; --card: 222.2 84% 4.9%; --card-foreground: 210 40% 98%; --popover: 222.2 84% 4.9%; --popover-foreground: 210 40% 98%; --primary: 210 40% 98%; --primary-foreground: 222.2 47.4% 11.2%; --secondary: 217.2 32.6% 17.5%; --secondary-foreground: 210 40% 98%; --muted: 217.2 32.6% 17.5%; --muted-foreground: 215 20.2% 65.1%; --accent: 217.2 32.6% 17.5%; --accent-foreground: 210 40% 98%; --destructive: 0 62.8% 30.6%; --destructive-foreground: 210 40% 98%; --border: 217.2 32.6% 17.5%; --input: 217.2 32.6% 17.5%; --ring: 212.7 26.8% 83.9%; + --radius: 0.5rem; --chart-1: 220 70% 50%; --chart-2: 160 60% 45%; --chart-3: 30 80% 55%; --chart-4: 280 65% 60%; --chart-5: 340 75% 55%; }packages/design-system/src/values.ts (3)
25-32: LGTM with a minor suggestion: Programming values are well-structured.The
programmingValuesconstant maintains the established structure and covers popular programming languages. For consistency, consider using lowercase for allvalueproperties, including "csharp".- { value: 'csharp', label: 'C#' }, + { value: 'c-sharp', label: 'C#' },This change would align with the pattern used for other languages while keeping the label readable.
43-52: LGTM with a minor suggestion: Group values provide flexible categorization.The
groupValuesconstant effectively organizes the previously defined constants into logical groups. The use of optional properties likeshowSeparatorandlabelallows for customized presentation.For consistency, consider adding labels to all groups:
- { items: fruitsValues }, + { items: fruitsValues, label: 'Fruits' }, - { items: vegetablesValues, showSeparator: true }, + { items: vegetablesValues, showSeparator: true, label: 'Vegetables' }, { items: transportValues, showSeparator: true, label: 'Transport' }, { items: programmingValues, showSeparator: true, label: 'Programming Languages', },This change would provide a consistent structure across all groups and improve the overall organization of the select field options.
1-63: Consider TypeScript integration and potential for abstraction.The overall structure of the file is well-organized and logical, progressing from individual value arrays to grouped value arrays. To further improve the code:
Consider using TypeScript to add type safety to these constants. This would help catch potential errors early in the development process.
Explore the possibility of using enums for some of these values, especially for the categories that are unlikely to change frequently (e.g., transport modes).
If these values are used across multiple components, consider creating a dedicated types file to centralize the type definitions.
Example of how this might look with TypeScript:
enum FruitType { Apple = 'apple', Banana = 'banana', // ... } interface SelectOption { value: string; label: string; disabled?: boolean; } const fruitsValues: SelectOption[] = [ { value: FruitType.Apple, label: 'Apple' }, // ... ]; // Similar enums and type-safe arrays for other categoriesThese changes would enhance type safety and make the code more robust and maintainable.
packages/design-system/src/ui/select.stories.tsx (1)
16-27: LGTM: Default story demonstrates basic Select functionality.The Default story showcases a basic Select component with appropriate structure and common theme options.
Consider adding an
aria-labelto the Select component for improved accessibility. For example:- <Select> + <Select aria-label="Choose theme">packages/design-system/src/ui/select.tsx (2)
15-18: Approve changes with a minor suggestion for prop namingThe implementation of the
hideIconprop in theSelectTriggercomponent looks good. It provides more flexibility for consumers of the component while maintaining backward compatibility. The conditional rendering is implemented correctly.Consider renaming the
hideIconprop toshowIconand inverting the logic. This would make the prop's purpose more intuitive:- hideIcon?: boolean + showIcon?: boolean- ({ className, children, hideIcon = false, ...props }, ref) => ( + ({ className, children, showIcon = true, ...props }, ref) => (- {!hideIcon && ( + {showIcon && (This change would make the default behavior more explicit (showing the icon by default) and might be easier to understand for other developers.
Also applies to: 28-32
Line range hint
1-180: Overall assessment: Changes are well-implemented and aligned with PR objectivesThe introduction of the
hideIconprop to theSelectTriggercomponent is a good addition that enhances the flexibility of the select field. The implementation is clean, maintains backward compatibility, and follows React best practices. The changes are well-contained and don't introduce any breaking changes to the existing functionality.Consider adding unit tests for the new functionality to ensure the
hideIconprop works as expected in different scenarios.packages/design-system/src/Select.tsx (2)
155-170: Ensure uniqueness of keys when mapping overitems.When rendering
SelectItemcomponents, usingkey={${item.value}-${ix}}may not guarantee unique and stable keys, especially ifitem.valueis not unique or if the list reorders. To ensure keys are unique and stable across renders, consider using a unique identifier likeitem.idif available.Modify the key as follows:
- key={`${item.value}-${ix}`} + key={item.id || `${item.value}-${ix}`}
181-197: Ensure uniqueness of keys when mapping overgroup.items.Similar to the previous comment, when rendering
SelectItemcomponents within groups, usingkey={${item.value}-${item_ix}}may not ensure uniqueness and stability. Consider incorporating the group identifier or usingitem.idif available.You could adjust the key as follows:
- key={`${item.value}-${item_ix}`} + key={item.id || `${group.label || 'group'}-${ix}-item-${item_ix}`}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/design-system/.ladle/head.html (1 hunks)
- packages/design-system/src/Select.stories.tsx (2 hunks)
- packages/design-system/src/Select.tsx (3 hunks)
- packages/design-system/src/forms/FormikSelectField.stories.tsx (1 hunks)
- packages/design-system/src/forms/SelectField.stories.tsx (1 hunks)
- packages/design-system/src/tailwind.css (1 hunks)
- packages/design-system/src/ui/select.stories.tsx (1 hunks)
- packages/design-system/src/ui/select.tsx (2 hunks)
- packages/design-system/src/values.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/design-system/src/forms/SelectField.stories.tsx
🔇 Additional comments (16)
packages/design-system/src/tailwind.css (1)
26-26: Confirm the intentional increase of the base radius.The base radius value has been increased from 0.3rem to 0.5rem. This change will affect the border radius of various components throughout the design system.
Please confirm that this change is intentional and aligns with the new select field implementation mentioned in the PR title. Additionally, consider the following:
- Has this change been approved by the design team?
- Are there any components that might need adjustments due to this increased radius?
- Does this change maintain consistency with other radius values in the system?
To help verify the impact, you can run the following script to check for other radius-related variables or usages:
This will help ensure that the change is consistent across the design system.
packages/design-system/src/values.ts (4)
1-8: LGTM: Fruit values are well-structured and consistent.The
fruitsValuesconstant is well-defined with a clear structure. Each entry has a lowercasevalueand a capitalizedlabel, which is a good practice for consistency and readability.
9-16: LGTM: Vegetable values maintain consistency with fruit values.The
vegetablesValuesconstant follows the same structure asfruitsValues, maintaining consistency across the file. The lowercasevalueand capitalizedlabelpattern is preserved.
17-24: LGTM: Transport values are consistent and cover various modes.The
transportValuesconstant maintains the established structure and naming conventions. It provides a good variety of transportation modes, enhancing the usability of the select field.
54-63: LGTM: Disabled group values maintain consistency with regular group values.The
groupValuesDisabledconstant effectively mirrors the structure ofgroupValues, providing a consistent approach for handling disabled options. This consistency is beneficial for code readability and maintenance.As suggested for
groupValues, consider adding labels to all groups here as well for improved consistency and organization.packages/design-system/.ladle/head.html (1)
41-41: Confirm intentional increase in border radius and verify impact.The
--radiusvariable has been increased from 0.3rem to 0.5rem, which represents a significant change in the roundness of corners for elements using this variable. This change could have a noticeable impact on the appearance of UI components throughout the design system.Please confirm the following:
- Is this change intentional and aligned with the design goals?
- Have you verified the impact on existing components to ensure they still look as intended with the increased radius?
- Does this change require updates to design documentation or style guides?
To assist in verifying the impact, you may want to run the following command to find all occurrences of
var(--radius)in your codebase:This will help identify all components that might be affected by this change.
✅ Verification successful
Increase in
--radiusconfirmed with limited scope.The
--radiusvariable has been updated from0.3remto0.5remintailwind.config.js, affecting thelg,md, andsmborder-radius utilities. This change will enhance the roundness of elements using these Tailwind classes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type css --type html --type javascript 'var\(--radius\)'Length of output: 98
Script:
#!/bin/bash # Searching for occurrences of 'var(--radius)' in CSS, HTML, and JavaScript files rg --type css --type html --type js 'var\(--radius\)'Length of output: 292
packages/design-system/src/ui/select.stories.tsx (1)
1-14: LGTM: Imports and default export are well-structured.The imports for the Select components and Ladle story types are appropriate. The default export correctly sets the title for the story and uses 'satisfies' for type safety.
packages/design-system/src/Select.stories.tsx (4)
Line range hint
1-200: Clarify the removal of the Small storyThe AI summary indicates that the Small story has been removed entirely from this file. While this might be appropriate if the Small variant is no longer needed or supported, it's important to ensure that this doesn't negatively impact test coverage or documentation of component variants.
Could you please clarify the reasoning behind removing the Small story? If it's still a supported variant, consider keeping it for comprehensive documentation and testing. If it's no longer needed, please confirm that this is intentional and aligns with the current design system requirements.
Line range hint
1-200: Summary of changes and recommendations
The refactoring of constant definitions to a separate
values.tsfile improves code organization and potential reuse. This change is approved, but please verify the contents of the new file as suggested.The UI adjustment in the Styled story (adding
text-lgto the trigger class) is approved. Please verify the visual impact using the suggested screenshot comparison.The removal of the Small story requires clarification. Please provide reasoning for this removal and confirm if it aligns with current design system requirements.
Overall, these changes appear to improve the code structure and maintainability. Once the points above are addressed, this PR should be ready for merging.
152-152: Approve UI adjustment and suggest visual verificationThe addition of
text-lgto the trigger class in the Styled story is a minor UI adjustment that likely improves readability or visual appeal. This change is appropriately localized to this specific story.To ensure the visual impact is as intended, please run the following commands to capture a screenshot of the Styled story before and after this change:
#!/bin/bash # Description: Capture screenshots of the Styled story for visual comparison # Test: Use Storybook's screenshot functionality to capture the Styled story npx storybook-chrome-screenshot -c .storybook -s packages/design-system/src/Select.stories.tsx:StyledPlease review the generated screenshots to confirm that the text size increase in the trigger element is visually appropriate and doesn't cause any layout issues.
3-3: Approve import changes and verify new moduleThe refactoring to import
fruitsValuesandgroupValuesfrom a separate file is a good practice for code organization and potential reuse. This change aligns with the principle of separation of concerns.To ensure the correctness of this change, please run the following script to verify the contents of the new
values.tsfile:✅ Verification successful
Import Changes Verified Successfully
The
fruitsValuesandgroupValuesare correctly exported fromvalues.tsand properly imported inSelect.stories.tsx. The refactoring enhances code organization without introducing any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the contents of the new values.ts file # Test: Check if the file exists and contains the expected exports cat packages/design-system/src/values.tsLength of output: 174
Script:
#!/bin/bash # Description: Verify the contents of the new values.ts file by displaying its contents. # Test: Check if the file exists and output its contents cat packages/design-system/src/values.tsLength of output: 2061
packages/design-system/src/forms/FormikSelectField.stories.tsx (3)
Line range hint
1-359: Usage of imported values is correct and unchanged.I've verified that the usage of the imported values (fruitsValues, groupValues, groupValuesDisabled, programmingValuesDisabled) remains consistent throughout the file. No changes were needed in the stories due to the import change.
Line range hint
1-359: Summary: Minor import change with no functional impact.The only change in this file is the update to the import statement on line 8. All stories and their functionality remain unchanged. This refactoring improves code organization without affecting the component's behavior or the storybook examples.
8-8: LGTM! Verify the new import path.The change to import from '../values' instead of '../Select.stories' looks good. This refactoring likely improves code organization and reusability.
To ensure the new import path is correct, please run the following script:
This script will check if the '../values' file exists and if it exports the required values.
packages/design-system/src/Select.tsx (2)
108-111:useEffectcorrectly updatesshortLabelbased on dependencies.The
useEffecthook effectively recalculatesshortLabelwhenevervalue,items, orgroupschange. The usage and dependencies are appropriate.
Line range hint
117-124: Proper handling ofonOpenChangeandonBlurevents.The
onOpenChangehandler correctly updates theopenstate and callsonBlurwhen the select menu closes. This ensures that any blur-related logic is executed appropriately.
| export const programmingValuesDisabled = [ | ||
| { value: 'javascript', label: 'JavaScript (disabled)', disabled: true }, | ||
| { value: 'typescript', label: 'TypeScript' }, | ||
| { value: 'python', label: 'Python (disabled)', disabled: true }, | ||
| { value: 'ruby', label: 'Ruby' }, | ||
| { value: 'java', label: 'Java (disabled)' }, | ||
| { value: 'csharp', label: 'C#' }, | ||
| ] |
There was a problem hiding this comment.
Fix inconsistency in disabled entries and consider UI implications.
The programmingValuesDisabled constant has a few issues:
- The "Java" entry is labeled as disabled but doesn't have the
disabled: trueproperty. - Appending "(disabled)" to labels might be redundant if the UI can visually indicate disabled status.
Consider the following changes:
- { value: 'javascript', label: 'JavaScript (disabled)', disabled: true },
+ { value: 'javascript', label: 'JavaScript', disabled: true },
{ value: 'typescript', label: 'TypeScript' },
- { value: 'python', label: 'Python (disabled)', disabled: true },
+ { value: 'python', label: 'Python', disabled: true },
{ value: 'ruby', label: 'Ruby' },
- { value: 'java', label: 'Java (disabled)' },
+ { value: 'java', label: 'Java', disabled: true },
{ value: 'csharp', label: 'C#' },These changes ensure consistency across all entries and rely on the UI to indicate disabled status rather than modifying the labels.
📝 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.
| export const programmingValuesDisabled = [ | |
| { value: 'javascript', label: 'JavaScript (disabled)', disabled: true }, | |
| { value: 'typescript', label: 'TypeScript' }, | |
| { value: 'python', label: 'Python (disabled)', disabled: true }, | |
| { value: 'ruby', label: 'Ruby' }, | |
| { value: 'java', label: 'Java (disabled)' }, | |
| { value: 'csharp', label: 'C#' }, | |
| ] | |
| export const programmingValuesDisabled = [ | |
| { value: 'javascript', label: 'JavaScript', disabled: true }, | |
| { value: 'typescript', label: 'TypeScript' }, | |
| { value: 'python', label: 'Python', disabled: true }, | |
| { value: 'ruby', label: 'Ruby' }, | |
| { value: 'java', label: 'Java', disabled: true }, | |
| { value: 'csharp', label: 'C#' }, | |
| ] |
| export const Groups: Story = () => ( | ||
| <Select> | ||
| <SelectTrigger className="w-[280px]"> | ||
| <SelectValue placeholder="Select a timezone" /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectGroup> | ||
| <SelectLabel>North America</SelectLabel> | ||
| <SelectItem value="est">Eastern Standard Time (EST)</SelectItem> | ||
| <SelectItem value="cst">Central Standard Time (CST)</SelectItem> | ||
| <SelectItem value="mst">Mountain Standard Time (MST)</SelectItem> | ||
| <SelectItem value="pst">Pacific Standard Time (PST)</SelectItem> | ||
| <SelectItem value="akst">Alaska Standard Time (AKST)</SelectItem> | ||
| <SelectItem value="hst">Hawaii Standard Time (HST)</SelectItem> | ||
| </SelectGroup> | ||
| <SelectGroup> | ||
| <SelectLabel>Europe & Africa</SelectLabel> | ||
| <SelectItem value="gmt">Greenwich Mean Time (GMT)</SelectItem> | ||
| <SelectItem value="cet">Central European Time (CET)</SelectItem> | ||
| <SelectItem value="eet">Eastern European Time (EET)</SelectItem> | ||
| <SelectItem value="west"> | ||
| Western European Summer Time (WEST) | ||
| </SelectItem> | ||
| <SelectItem value="cat">Central Africa Time (CAT)</SelectItem> | ||
| <SelectItem value="eat">East Africa Time (EAT)</SelectItem> | ||
| </SelectGroup> | ||
| <SelectGroup> | ||
| <SelectLabel>Asia</SelectLabel> | ||
| <SelectItem value="msk">Moscow Time (MSK)</SelectItem> | ||
| <SelectItem value="ist">India Standard Time (IST)</SelectItem> | ||
| <SelectItem value="cst_china">China Standard Time (CST)</SelectItem> | ||
| <SelectItem value="jst">Japan Standard Time (JST)</SelectItem> | ||
| <SelectItem value="kst">Korea Standard Time (KST)</SelectItem> | ||
| <SelectItem value="ist_indonesia"> | ||
| Indonesia Central Standard Time (WITA) | ||
| </SelectItem> | ||
| </SelectGroup> | ||
| <SelectGroup> | ||
| <SelectLabel>Australia & Pacific</SelectLabel> | ||
| <SelectItem value="awst"> | ||
| Australian Western Standard Time (AWST) | ||
| </SelectItem> | ||
| <SelectItem value="acst"> | ||
| Australian Central Standard Time (ACST) | ||
| </SelectItem> | ||
| <SelectItem value="aest"> | ||
| Australian Eastern Standard Time (AEST) | ||
| </SelectItem> | ||
| <SelectItem value="nzst">New Zealand Standard Time (NZST)</SelectItem> | ||
| <SelectItem value="fjt">Fiji Time (FJT)</SelectItem> | ||
| </SelectGroup> | ||
| <SelectGroup> | ||
| <SelectLabel>South America</SelectLabel> | ||
| <SelectItem value="art">Argentina Time (ART)</SelectItem> | ||
| <SelectItem value="bot">Bolivia Time (BOT)</SelectItem> | ||
| <SelectItem value="brt">Brasilia Time (BRT)</SelectItem> | ||
| <SelectItem value="clt">Chile Standard Time (CLT)</SelectItem> | ||
| </SelectGroup> | ||
| </SelectContent> | ||
| </Select> | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Duplicate Time Zone Abbreviation Detected
The time zone abbreviation 'CST' is used for both Central Standard Time (CST) and China Standard Time (CST). This duplication can cause confusion for users selecting a time zone. Consider using unique identifiers for each time zone to ensure clarity, such as cst_america and cst_china.
🔗 Analysis chain
LGTM: Groups story demonstrates complex Select functionality with logical grouping.
The Groups story showcases a more complex Select component with well-structured grouped options for time zones. The grouping by region enhances user experience and readability.
Consider the following improvements:
- Add an
aria-labelto the Select component for improved accessibility:- <Select> + <Select aria-label="Select a timezone">
- To avoid confusion with duplicate abbreviations, consider using unique identifiers for values:
- <SelectItem value="cst">Central Standard Time (CST)</SelectItem> + <SelectItem value="cst_america">Central Standard Time (CST)</SelectItem>
- For consistency, consider using the same format for all time zone values (e.g., lowercase abbreviations):
- <SelectItem value="ist_indonesia"> + <SelectItem value="wita">
To ensure all time zone abbreviations are unique, run the following script:
If the script returns any results, consider updating the duplicate abbreviations to ensure uniqueness.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for duplicate time zone abbreviations in select.stories.tsx
# Test: Extract time zone abbreviations and check for duplicates
grep -oP '(?<=\().*?(?=\))' packages/design-system/src/ui/select.stories.tsx | sort | uniq -d
Length of output: 99
| <SelectValue placeholder={placeholder} key={shortLabel}> | ||
| {shortLabel} | ||
| </SelectValue> | ||
| ) : ( | ||
| <SelectValue placeholder={placeholder} key={shortLabel} /> |
There was a problem hiding this comment.
Remove unnecessary key prop from SelectValue components.
The key prop is typically used when rendering lists of components to help React identify which items have changed. In this context, using key={shortLabel} on SelectValue is unnecessary and could potentially cause issues if shortLabel is undefined or changes frequently. It's advisable to remove the key prop here.
Apply this diff to remove the key prop:
{shortLabel ? (
- <SelectValue placeholder={placeholder} key={shortLabel}>
+ <SelectValue placeholder={placeholder}>
{shortLabel}
</SelectValue>
) : (
- <SelectValue placeholder={placeholder} key={shortLabel} />
+ <SelectValue placeholder={placeholder} />
)}📝 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.
| <SelectValue placeholder={placeholder} key={shortLabel}> | |
| {shortLabel} | |
| </SelectValue> | |
| ) : ( | |
| <SelectValue placeholder={placeholder} key={shortLabel} /> | |
| <SelectValue placeholder={placeholder}> | |
| {shortLabel} | |
| </SelectValue> | |
| ) : ( | |
| <SelectValue placeholder={placeholder} /> |
| 'w-[15rem] text-base', | ||
| disabled && 'bg-uzh-grey-20 opacity-70', | ||
| basic && '[all:_unset]', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the SelectTrigger width more flexible.
The hardcoded width 'w-[15rem]' may limit the component's responsiveness and adaptability. To enhance flexibility, consider allowing the width to be customizable via props or using a relative unit like 'w-full' to make the component responsive.
You could adjust the className as follows:
className={twMerge(
- 'w-[15rem] text-base',
+ 'w-full text-base',
disabled && 'bg-uzh-grey-20 opacity-70',
basic && '[all:_unset]',
className?.trigger
)}📝 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.
| 'w-[15rem] text-base', | |
| disabled && 'bg-uzh-grey-20 opacity-70', | |
| basic && '[all:_unset]', | |
| 'w-full text-base', | |
| disabled && 'bg-uzh-grey-20 opacity-70', | |
| basic && '[all:_unset]', |
Summary by CodeRabbit
Release Notes
New Features
--radiusfor enhanced element styling.Selectcomponent stories, showcasing default and grouped selections.hideIconprop toSelectTriggerfor customizable icon visibility.Bug Fixes
Refactor
Selectcomponent to utilize new internal components, enhancing flexibility.