storybook audit fixes for typography,hamburger and sidebar #142
Conversation
WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on enhancing typography styles in the Changes
Possibly related PRs
Suggested reviewers
Poem
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: 35
🧹 Outside diff range and nitpick comments (21)
react/example/public/index.html (1)
12-12: Consider adding version constraints.To prevent unintended updates and ensure consistent behavior, consider using version constraints (e.g., exact version or caret/tilde ranges) in package.json instead of directly referencing unpkg URLs.
This approach would:
- Make version management more maintainable
- Ensure consistent versions across environments
- Allow for proper dependency tracking
react/ui-components/src/atoms/stories/LinkTypography.stories.js (1)
4-8: Add component documentation and controls.Consider enhancing the story configuration with:
- Component description using the
parameters.docs.descriptionfield- Common props in
argTypesfor interactive documentationexport default { title: "Foundations/Typography/Link", + parameters: { + docs: { + description: { + component: 'Typography component for links in different sizes: L (20pts), S (16pts), and XS (14pts).' + } + } + }, argTypes: { + className: { + description: 'Additional CSS class names', + control: 'text' + }, + children: { + description: 'Link content', + control: 'text' + } }, };react/ui-components/src/atoms/stories/ButtonTypography.stories.js (1)
1-88: Enhance accessibility and internationalization.Consider the following improvements:
- Add proper ARIA labels and roles
- Use semantic HTML elements where appropriate
- Extract text content for internationalization
Example improvements:
+import { useTranslation } from 'react-i18next'; const ButtonTypography = ({ size }) => { + const { t } = useTranslation(); const sizeMap = { - large: { text: "button large / 20pts", className: "large" }, + large: { text: t('typography.button.large.label'), className: "large" }, // ... other sizes }; return ( <div + role="presentation" + aria-label={t('typography.button.preview')} // ... other props >react/ui-components/src/molecules/stories/Footer.stories.js (1)
6-6: Resolve component naming and categorization inconsistencies.There are several architectural inconsistencies that need to be addressed:
- The story is titled "Footer" but demonstrates the
ActionBarcomponent- The component is imported from "atoms" but categorized under "Molecules"
- The file name
Footer.stories.jsdoesn't match the actual component being demonstratedConsider the following improvements:
- Rename the file to
ActionBar.stories.jsto match the component- Either move the story back to "Atoms" or move the
ActionBarcomponent to the molecules directory- Update the story title accordingly based on the chosen categorization
This will improve maintainability and make the component easier to discover in Storybook.
react/ui-components/src/atoms/stories/HeadingTypography.stories.js (1)
10-19: Improve style object formatting and positioning.
- The use of absolute positioning might cause layout issues in different viewport sizes
- Style properties should have consistent spacing
const style = { - position: "absolute", - top: "50%", - left: "50%", + margin: "2rem auto", color: "#363636", display: "flex", justifyContent: "center", - transform: "translate(-50%, -50%)", - lineHeight:"normal" + lineHeight: "normal" };react/ui-components/src/atoms/BreadCrumb.js (2)
Line range hint
103-112: Add missing PropType validation and improve type definitions.The new
externalPathproperty is not defined in PropTypes, and some existing prop validations could be more specific.Update the PropTypes definition:
BreadCrumb.propTypes = { - crumbs: PropTypes.array, + crumbs: PropTypes.arrayOf(PropTypes.shape({ + content: PropTypes.node.isRequired, + path: PropTypes.string, + externalPath: PropTypes.bool, + show: PropTypes.bool, + icon: PropTypes.node, + isBack: PropTypes.bool, + count: PropTypes.number + })).isRequired, className: PropTypes.string, style: PropTypes.object, spanStyle: PropTypes.object, customSeparator: PropTypes.element, maxItems: PropTypes.number, itemsAfterCollapse: PropTypes.number, itemsBeforeCollapse: PropTypes.number, expandText: PropTypes.string };
Line range hint
64-98: Improve accessibility and React key handling in the breadcrumb list.The current implementation has some accessibility and React key issues that should be addressed.
Consider these improvements:
- Add proper ARIA attributes for breadcrumb navigation
- Replace Fragment with proper key handling
- Add proper role attributes
- <> + <Fragment key={`crumb-${ci}`}> <li - key={ci} style={props?.itemStyle} - className="digit-bread-crumb--item" + className="digit-bread-crumb--item" + role="listitem" > {/* ... existing content ... */} </li> - </> + </Fragment>Also, consider adding these attributes to the outer
olelement:<ol className={`digit-bread-crumb ${ props?.className ? props?.className : "" }`} + role="navigation" + aria-label="Breadcrumb" style={props?.style} >react/ui-components/src/molecules/stories/DarkThemeHamburger.stories.js (1)
15-15: Fix inconsistent spacing in object properties.The
isSearchableproperty has inconsistent spacing. Maintain consistent spacing in object properties.- isSearchable:{control:"boolean"}, + isSearchable: { control: "boolean" },react/ui-components/src/molecules/stories/LightThemeHamburger.stories.js (1)
9-24: Improve argTypes organization and consistency.Consider grouping related props together and maintaining consistent spacing:
argTypes: { + // Appearance theme: { control: "select", options: ["dark", "light"] }, variant: { control: "select", options: ["primary", "secondary"] }, transitionDuration: { control: "number" }, + // Content items: { control: "object" }, usermanuals: { control: "object" }, userManualLabel: { control: "text" }, profile: { control: "text" }, + // Behavior isSearchable: { control: "boolean" }, hideUserManuals: { control: "boolean" }, reopenOnLogout: { control: "boolean" }, closeOnClickOutside: { control: "boolean" }, + // Events onSelect: { action: "onChange" }, onLogout: { action: "onChange" }, onOutsideClick: { action: "onChange" } }react/ui-components/src/molecules/stories/LightThemeHeader.stories.js (1)
Line range hint
1-358: Consider enhancing story documentation.While the component stories demonstrate various configurations well, consider adding JSDoc comments to describe:
- The purpose of each story variant
- The expected behavior of the interactive elements
- The use cases for different configurations
This would improve the documentation value of the storybook.
Example enhancement:
+/** + * Default light theme header with basic configuration. + * Shows the header with city name and language selector. + */ export const LightThemeHeader = Template.bind({}); +/** + * Light theme header with dropdown menu options. + * Demonstrates the header with multiple dropdown fields for city selection, + * language preference, and user profile actions. + */ export const WithActionFields = Template.bind({});react/ui-components/src/molecules/stories/DarkThemeHeader.stories.js (1)
192-204: Update default theme in commonArgs.Since this file is specifically for dark theme stories, consider updating the default theme.
const commonArgs = { img: "", className: "", style: {}, - theme: "light", + theme: "dark", setImageToLeft: false,react/ui-components/src/atoms/stories/Dropdown.stories.js (2)
Line range hint
7-31: Update argTypes to match new naming convention and remove deprecated options.The configuration needs to be aligned with the new naming convention and removed features:
- The
typeoptions still include "multiselectdropdown" despite multi-select functionality being removed- The
variantoptions use old naming (e.g., "nesteddropdown") while story exports use new naming (e.g., "Categorical")Apply this diff to update the configuration:
type: { control: "select", options: [ - "dropdown", "multiselectdropdown" + "dropdown" ]}, variant: { control: "select", options: [ - "nesteddropdown", - "treedropdown", - "nestedtextdropdown", - "profiledropdown", - "profilenestedtext", + "categorical", + "tree", + "nestedText", + "profile", + "profileNestedText", ], },
Line range hint
254-399: Enhance story documentation and align variant names.While the story exports are well-organized, consider these improvements:
- Add descriptions to each story export to explain its purpose and use case
- Ensure variant names in story exports match those in argTypes
Example enhancement for the Basic story:
export const Basic = Template.bind({}); +Basic.parameters = { + docs: { + description: { + story: 'A simple dropdown component with basic functionality. Use this variant for standard single-select dropdowns.', + }, + }, +}; Basic.args = { ...commonArgs, type: "dropdown", };react/ui-components/src/atoms/MobileSidebar.js (1)
Line range hint
348-354: Verify isSearchable prop documentationThe
isSearchableprop has been added to propTypes, which formalizes the existing search functionality. Consider adding JSDoc comments to document this feature for better developer experience.Add documentation above the propTypes:
/** * @property {boolean} isSearchable - When true, enables search functionality for items with children */react/ui-components/src/atoms/stories/MultiselectDropdown.stories.js (4)
1-31: Consider simplifying the component setup.The
QueryClientProvidersetup seems unnecessary for a presentational Storybook story since the dropdown component doesn't appear to make any queries. Consider removing it unless there's a specific requirement.-import { QueryClient, QueryClientProvider } from "react-query"; -const queryClient = new QueryClient(); const Template = (args) => ( - <QueryClientProvider client={queryClient}> <FieldV1 {...args} /> - </QueryClientProvider> );
42-180: Improve mock data organization and naming conventions.
The mock data variable names are inconsistent:
gendersOptions(camelCase)OptionsWithIcons(PascalCase)NestedTextOptionWithIcons(PascalCase)The nested options have inconsistent naming patterns:
- "Option A" vs "Option 1" vs "Option1"
Consider standardizing the naming conventions:
-const OptionsWithIcons = [ +const optionsWithIcons = [ -const NestedTextOptionWithIcons = [ +const nestedTextOptionsWithIcons = [
211-241: Refine common arguments configuration.
- The
nameproperty in populators is set to "genders" but used for generic options- Boolean flags like
addSelectAllCheckandaddCategorySelectAllCheckcould be more descriptive- There are typos in some property names:
isSearchable:true(missing space after colon)clearLabel:"Clear All"(missing space after colon)Consider these improvements:
populators: { - name: "genders", + name: "options", defaultValue: "FEMALE", optionsCustomStyle: {}, optionsKey: "name", options: gendersOptions, showIcon: false, - isSearchable:true, - clearLabel:"Clear All", + isSearchable: true, + clearLabel: "Clear All", - addSelectAllCheck:false, - addCategorySelectAllCheck:false, + enableSelectAll: false, + enableCategorySelectAll: false,
1-404: Consider adding stories for edge cases.The current stories cover the main use cases well, but consider adding stories for:
- Error states (invalid options, network errors)
- Loading states
- Empty states (no options)
- Maximum selection limit
- Custom option rendering
Would you like me to help generate additional stories for these edge cases?
react/ui-components/src/atoms/MultiSelectDropdown.js (3)
712-712: LGTM! Consider documenting thehideCloseprop.The addition of
hideClose={false}maintains consistent behavior with other components. However, since this is a new property, it would be helpful to document it in the PropTypes.Add the following to the PropTypes definition:
MultiSelectDropdown.propTypes = { options: PropTypes.array.isRequired, optionsKey: PropTypes.string.isRequired, selected: PropTypes.array, onSelect: PropTypes.func.isRequired, defaultLabel: PropTypes.string, defaultUnit: PropTypes.string, BlockNumber: PropTypes.number, isOBPSMultiple: PropTypes.bool, props: PropTypes.object, isPropsNeeded: PropTypes.bool, ServerStyle: PropTypes.object, config: PropTypes.object, + // Document that this prop is passed to the Chip component + hideClose: PropTypes.bool, };
Line range hint
391-449: Consider refactoring keyboard navigation logic for better maintainability.The
keyChangefunction directly manipulates DOM elements and contains complex scrolling logic. Consider extracting this into a custom hook for better reusability and testability.Here's a suggested approach:
// useKeyboardNavigation.js const useKeyboardNavigation = (options, optionIndex, setOptionIndex) => { const handleKeyDown = (e, scrollRef) => { if (options.length === 0) return; switch(e.key) { case "ArrowDown": e.preventDefault(); const nextIndex = optionIndex + 1 >= options.length ? 0 : optionIndex + 1; setOptionIndex(nextIndex); scrollRef.current?.children[nextIndex]?.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); break; case "ArrowUp": e.preventDefault(); const prevIndex = optionIndex === 0 ? options.length - 1 : optionIndex - 1; setOptionIndex(prevIndex); scrollRef.current?.children[prevIndex]?.scrollIntoView({ behavior: 'smooth', block: 'nearest' }); break; } }; return handleKeyDown; };
Line range hint
32-95: Consider memoizing complex calculations and callbacks.The component re-renders frequently due to state changes. Memoizing callbacks and computed values would improve performance.
Consider these optimizations:
const memoizedFilteredOptions = useMemo(() => searchQuery?.length > 0 ? options?.filter(option => t(option[optionsKey]?.toString().toUpperCase()) .toLowerCase() .includes(searchQuery.toLowerCase()) ) : options, [searchQuery, options, optionsKey] ); const handleSelect = useCallback((e, option) => { if (isPropsNeeded) { onSelectToAddToQueue(e, option, props); } else if (isOBPSMultiple) { onSelectToAddToQueue(e, option, BlockNumber); } else { onSelectToAddToQueue(e, option); } }, [isPropsNeeded, isOBPSMultiple, BlockNumber, props]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
react/css/package.jsonis excluded by!**/*.jsonreact/example/package.jsonis excluded by!**/*.jsonreact/modules/core/package.jsonis excluded by!**/*.jsonreact/modules/sample/package.jsonis excluded by!**/*.jsonreact/package.jsonis excluded by!**/*.jsonreact/ui-components/package.jsonis excluded by!**/*.json
📒 Files selected for processing (24)
- react/css/src/digitv2/index.scss (1 hunks)
- react/example/public/index.html (1 hunks)
- react/ui-components/.storybook/preview-head.html (1 hunks)
- react/ui-components/public/index.html (1 hunks)
- react/ui-components/src/atoms/BreadCrumb.js (2 hunks)
- react/ui-components/src/atoms/MobileSidebar.js (1 hunks)
- react/ui-components/src/atoms/MultiSelectDropdown.js (1 hunks)
- react/ui-components/src/atoms/UploadFile.js (1 hunks)
- react/ui-components/src/atoms/Uploader.js (1 hunks)
- react/ui-components/src/atoms/stories/BodyTypography.stories.js (1 hunks)
- react/ui-components/src/atoms/stories/ButtonTypography.stories.js (1 hunks)
- react/ui-components/src/atoms/stories/CaptionTypography.stories.js (1 hunks)
- react/ui-components/src/atoms/stories/Dropdown.stories.js (10 hunks)
- react/ui-components/src/atoms/stories/Hamburger.stories.js (0 hunks)
- react/ui-components/src/atoms/stories/HeadingTypography.stories.js (1 hunks)
- react/ui-components/src/atoms/stories/LabelTypography.stories.js (1 hunks)
- react/ui-components/src/atoms/stories/LinkTypography.stories.js (1 hunks)
- react/ui-components/src/atoms/stories/MultiselectDropdown.stories.js (1 hunks)
- react/ui-components/src/atoms/stories/Typography.stories.js (0 hunks)
- react/ui-components/src/molecules/stories/DarkThemeHamburger.stories.js (1 hunks)
- react/ui-components/src/molecules/stories/DarkThemeHeader.stories.js (1 hunks)
- react/ui-components/src/molecules/stories/Footer.stories.js (1 hunks)
- react/ui-components/src/molecules/stories/LightThemeHamburger.stories.js (1 hunks)
- react/ui-components/src/molecules/stories/LightThemeHeader.stories.js (2 hunks)
💤 Files with no reviewable changes (2)
- react/ui-components/src/atoms/stories/Hamburger.stories.js
- react/ui-components/src/atoms/stories/Typography.stories.js
✅ Files skipped from review due to trivial changes (2)
- react/ui-components/.storybook/preview-head.html
- react/ui-components/public/index.html
🧰 Additional context used
🪛 Biome
react/ui-components/src/molecules/stories/DarkThemeHeader.stories.js
[error] 54-76: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 80-90: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 91-101: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 102-112: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 116-126: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 127-137: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 138-153: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 157-167: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 168-178: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 179-189: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (14)
react/example/public/index.html (1)
12-12: Verify CSS package version consistency across files.The CSS package version has been updated to 0.0.2-beta.40. Let's verify this change is consistent across all related files.
✅ Verification successful
CSS package version is consistent across active imports
The version
0.0.2-beta.40is consistently used in both active imports:
- react/example/public/index.html
- react/ui-components/public/index.html
There is one commented out reference to version
1.5.41which doesn't affect the runtime behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any inconsistencies in the CSS package version across HTML files # Test: Search for any references to the CSS package to ensure consistent versioning rg --type html "@egovernments/digit-ui-components-css@" -A 1 -B 1Length of output: 942
react/ui-components/src/atoms/stories/LinkTypography.stories.js (1)
30-30: Verify typography classes exist in CSS.Let's ensure the typography classes used in the stories are properly defined in the CSS files.
Also applies to: 53-53, 76-76
✅ Verification successful
Typography classes are properly defined in the codebase
The typography classes (
link-L,link-S,link-XS) used in the stories are correctly defined in:
- Primary source:
react/css/src/digitv2/typography.scss- Extended usage in button components:
react/css/src/digitv2/components/buttonsV2.scss🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if typography classes are defined in CSS files # Test: Search for link typography class definitions rg -t css "\.link-(L|S|XS)" react/Length of output: 1312
react/ui-components/src/atoms/stories/BodyTypography.stories.js (1)
30-30: Verify typography classes are properly defined.The story uses typography-related classes (
body-l,body-s,body-xs). Let's ensure these classes are properly defined in the CSS.Also applies to: 62-62, 94-94
✅ Verification successful
Based on the search results, I can see that the typography classes are well-defined in the codebase. The search shows:
- The main typography definitions are in
react/css/src/digitv2/typography.scss- The classes
body-l,body-s, andbody-xsare extensively used across many components- The typography classes follow a consistent pattern and are properly extended using SCSS's
@extend .typography.body-*syntaxTypography classes are properly defined and consistently used.
The story is using the correct typography classes that are well-established in the codebase's design system. No issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for typography class definitions echo "Searching for typography class definitions..." rg -l "body-(l|s|xs)" --type css # Check for any inconsistencies in class naming echo "Checking for class naming consistency..." rg "typography-?(story)?" --type cssLength of output: 57938
react/ui-components/src/atoms/stories/HeadingTypography.stories.js (1)
30-30: Verify className definitions in SCSS files.Let's ensure both
typographyandtypography-storyclasses are properly defined.Also applies to: 76-76, 122-122
✅ Verification successful
Classes are properly defined and used correctly
The
typographyandheading-xlclasses are properly defined inreact/css/src/digitv2/typography.scss. Thetypographyclass serves as a base class with nested modifiers includingheading-xl, which has appropriate responsive styles defined for different screen sizes. The implementation is consistent with the design system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for typography class definitions in SCSS files echo "Searching for typography classes..." rg -t scss "\.typography(-story)?\s*{"Length of output: 235
Script:
#!/bin/bash # Search for typography class definitions in SCSS files with fixed regex echo "Searching for typography classes..." rg -t scss "\.typography" # Also search for heading-xl class echo -e "\nSearching for heading-xl class..." rg -t scss "\.heading-xl" # List all scss files to ensure we're not missing anything echo -e "\nListing all SCSS files:" fd -e scssLength of output: 3342
Script:
#!/bin/bash # Search for typography classes in the typography.scss file echo "Checking typography.scss for class definitions..." cat react/css/src/digitv2/typography.scss # Search for any typography related classes in index.scss files echo -e "\nChecking index.scss files..." cat react/css/src/digitv2/index.scss cat react/css/src/index.scssLength of output: 42811
react/ui-components/src/atoms/UploadFile.js (2)
72-74: LGTM! The change maintains component consistency.The addition of
hideClose={false}aligns with similar updates in other components likeUploaderandMultiSelectDropdown, ensuring consistent behavior across the UI components.
72-72: Verify if hideClose prop is necessary.The explicit
hideClose={false}might be redundant if this is already the default behavior of the Chip component.Let's verify the Chip component's implementation:
✅ Verification successful
The explicit
hideClose={false}prop is necessaryThe Chip component has
hideClosedefaulting totruein its defaultProps, so explicitly setting it tofalseis required to show the close button. This is consistent with other components like Uploader and MultiSelectDropdown that also sethideClose={false}when they need the close functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if hideClose prop exists and its default value in Chip component # Search for hideClose prop definition in Chip component ast-grep --pattern 'PropTypes.{ $$$ hideClose: $_ $$$ }' # Search for defaultProps definition ast-grep --pattern 'Chip.defaultProps = { $$$ hideClose: $_ $$$ }'Length of output: 139
Script:
#!/bin/bash # Let's try a different approach to find the Chip component and its props # First, let's locate the Chip component file fd -t f "Chip.js" -x cat {} # Also search for any references to hideClose prop in the codebase rg "hideClose" -B 2 -A 2Length of output: 7114
react/ui-components/src/molecules/stories/DarkThemeHamburger.stories.js (2)
1-181: Overall structure looks good!The story file is well-organized and effectively demonstrates various use cases of the MobileSidebar component. The use of common arguments and clear story variations makes it easy to understand and maintain.
33-97: Replace placeholder paths with meaningful routes.All navigation paths are currently set to "/". Consider replacing these with actual routes that reflect the intended navigation structure.
react/ui-components/src/molecules/stories/LightThemeHeader.stories.js (2)
7-7: Appropriate reclassification from Atoms to Molecules.The move from "Atoms/Header" to "Molecules/Header/Light" better reflects the component's complexity, as it composes multiple atomic components (TopBar, Dropdown, SVG) into a more complex molecule. This aligns well with atomic design principles.
358-358: LGTM: Added missing semicolon.The addition of the semicolon improves code consistency.
react/ui-components/src/molecules/stories/DarkThemeHeader.stories.js (1)
1-51: LGTM! Well-structured Storybook configuration.The story configuration follows best practices with appropriate controls for all props.
react/ui-components/src/atoms/stories/Dropdown.stories.js (1)
Line range hint
1-399: Verify the impact of dropdown story changes.These story changes might affect:
- Documentation references to the old story names
- Other components that might be importing these stories
- Test files that might be using the old variant names
react/ui-components/src/atoms/MobileSidebar.js (1)
336-336: LGTM: Button size standardizationThe addition of
size="medium"to the Button component aligns with UI standardization efforts. This explicit size declaration improves consistency across the application.react/ui-components/src/atoms/Uploader.js (1)
400-400: LGTM! UX improvement for file removal.Making the close button always visible improves the discoverability of the file removal action.
|
check other code rabbit comments |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
react/ui-components/src/atoms/BreadCrumb.js (2)
Line range hint 114-123: Improve props configuration.
- The
successfulprop in defaultProps isn't used in the component. - Consider adding default values for the new pagination props.
BreadCrumb.propTypes = {
crumbs: PropTypes.array,
className: PropTypes.string,
style: PropTypes.object,
spanStyle: PropTypes.object,
customSeparator: PropTypes.element,
maxItems: PropTypes.number,
itemsAfterCollapse: PropTypes.number,
itemsBeforeCollapse: PropTypes.number,
expandText: PropTypes.string
};
BreadCrumb.defaultProps = {
- successful: true,
+ maxItems: null,
+ itemsAfterCollapse: Math.floor(maxItems / 2),
+ itemsBeforeCollapse: Math.ceil(maxItems / 2),
+ expandText: "..."
};Line range hint 1-124: Consider enhancing type safety for link properties.
To better align with the internal/external link distinction, consider adding proper type validation:
BreadCrumb.propTypes = {
- crumbs: PropTypes.array,
+ crumbs: PropTypes.arrayOf(
+ PropTypes.shape({
+ content: PropTypes.node.isRequired,
+ icon: PropTypes.node,
+ show: PropTypes.bool,
+ isBack: PropTypes.bool,
+ internalLink: PropTypes.string,
+ externalLink: PropTypes.string,
+ count: PropTypes.number
+ })
+ ),
// ... rest of the props
};🧰 Tools
🪛 Biome
[error] 64-64: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
react/ui-components/CHANGELOG.md (1)
Line range hint 1-400: Improve changelog maintenance practices.
Several areas of the changelog could be improved:
- Fill in the missing dates (YYYY-MM-DD) for beta versions
- Use consistent categorization across entries (Breaking Changes, Features, Bug Fixes, etc.)
- Provide more detailed descriptions of changes, including the rationale and impact
Consider using a changelog maintenance tool like standard-version to automate versioning and changelog generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- react/ui-components/CHANGELOG.md (1 hunks)
- react/ui-components/src/atoms/BreadCrumb.js (6 hunks)
🧰 Additional context used
🪛 Biome
react/ui-components/src/atoms/BreadCrumb.js
[error] 64-64: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (2)
react/ui-components/src/atoms/BreadCrumb.js (2)
26-29: Previous security concerns about URL validation remain unaddressed.
The current implementation of handleRedirect still lacks proper URL validation and sanitization.
79-86: External navigation implementation needs revision.
The component still uses Router's Link for external navigation instead of anchor tags as suggested in the previous review.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
react/ui-components/src/atoms/stories/BreadCrumb.stories.js (2)
Line range hint 1-206: Add story examples for externalLink usage.
The stories currently only demonstrate internalLink usage. Since the component now supports both internalLink and externalLink properties, it would be beneficial to add examples showing the externalLink functionality.
Consider adding a new story example:
export const WithExternalLinks = Template.bind({});
WithExternalLinks.args = {
crumbs: [
{
content: "Home",
show: true,
internalLink: "/home",
},
{
content: "Documentation",
show: true,
externalLink: "https://digit-ui.netlify.app/",
},
{
content: "External Page",
show: true,
externalLink: "https://example.com",
},
],
};Line range hint 6-23: Enhance component documentation with property descriptions.
The argTypes configuration would benefit from descriptions of the new internalLink and externalLink properties to help other developers understand their usage.
Add property descriptions to the argTypes:
argTypes: {
+ crumbs: {
+ description: 'Array of breadcrumb items. Each item can have either internalLink (for internal routing) or externalLink (for external URLs)',
+ control: { type: "object" },
+ },
// ... other properties
},📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- react/ui-components/src/atoms/stories/BreadCrumb.stories.js (6 hunks)
🔇 Additional comments (1)
react/ui-components/src/atoms/stories/BreadCrumb.stories.js (1)
49-49: Property name change from path to internalLink is consistently applied.
The migration from path to internalLink has been implemented consistently across all story examples. This aligns with the component's updated functionality.
Also applies to: 54-54, 59-59, 70-70, 75-75, 80-80, 85-85, 90-90, 102-102, 107-107, 112-112, 117-117, 122-122, 136-136, 141-141, 146-146, 151-151, 156-156, 167-167, 173-173, 179-179, 193-193, 198-198, 203-203
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
react/ui-components/src/atoms/BreadCrumb.js (2)
24-24: Consider adding null checks for optional props in dependency array.
The useEffect dependency array includes props?.expandText which uses optional chaining, but this could lead to unnecessary re-renders if the prop transitions between undefined and null.
Consider destructuring required props at the component start and providing default values:
const BreadCrumb = (props) => {
+ const {
+ crumbs,
+ maxItems,
+ itemsBeforeCollapse,
+ itemsAfterCollapse,
+ expandText = "..."
+ } = props;
// ... rest of the codeLine range hint 110-124: Update component props configuration.
The component has several issues in its props configuration:
- The
successfulprop in defaultProps isn't used anywhere in the component - Missing default values for new collapse-related props
Consider this update:
BreadCrumb.propTypes = {
crumbs: PropTypes.array.isRequired,
className: PropTypes.string,
style: PropTypes.object,
spanStyle: PropTypes.object,
customSeparator: PropTypes.element,
maxItems: PropTypes.number,
itemsAfterCollapse: PropTypes.number,
itemsBeforeCollapse: PropTypes.number,
expandText: PropTypes.string
};
BreadCrumb.defaultProps = {
- successful: true,
+ maxItems: null,
+ itemsAfterCollapse: 1,
+ itemsBeforeCollapse: 1,
+ expandText: "..."
};📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- react/ui-components/src/atoms/BreadCrumb.js (6 hunks)
🧰 Additional context used
🪛 Biome
react/ui-components/src/atoms/BreadCrumb.js
[error] 65-65: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
No description provided.