14063 add map breadcrumbs component to fe library #97
Conversation
WalkthroughThe recent changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Breadcrumbs
participant BreadcrumbItem
participant Ellipsis
User->>Breadcrumbs: Click on breadcrumb item
Breadcrumbs->>BreadcrumbItem: Trigger onClick
BreadcrumbItem-->>Breadcrumbs: Update active state
Breadcrumbs-->>User: Reflect active breadcrumb visually
Note over Breadcrumbs: If items exceed width, show Ellipsis
Breadcrumbs->>Ellipsis: Render hidden items
User->>Ellipsis: Click on hidden item
Ellipsis-->>Breadcrumbs: Update visible items
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
🎊 PR Preview c5d96fd has been successfully built and deployed to https://konturio-ui-preview-pr-97.surge.sh 🕐 Build time: 47.952s
🤖 By surge-preview |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (6)
- cosmos/cosmos.userdeps.js (2 hunks)
- packages/ui-kit/src/Breadcrumbs/BreadcrumbItem.module.css (1 hunks)
- packages/ui-kit/src/Breadcrumbs/BreadcrumbItem.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/Breadcrumbs.fixture.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/index.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/style.module.css (1 hunks)
Additional context used
Biome
packages/ui-kit/src/Breadcrumbs/BreadcrumbItem.tsx
[error] 28-28: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
packages/ui-kit/src/Breadcrumbs/index.tsx
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.(lint/style/useNumberNamespace)
[error] 43-43: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.(lint/style/useNumberNamespace)
Additional comments not posted (9)
packages/ui-kit/src/Breadcrumbs/style.module.css (1)
1-18: CSS Styles Look Good!The styles for the breadcrumbs container and breadcrumbs are well-defined and use CSS variables for consistency.
packages/ui-kit/src/Breadcrumbs/BreadcrumbItem.module.css (1)
1-34: CSS Styles Look Good!The styles for breadcrumb items, buttons, labels, and separators are well-structured and make good use of CSS variables.
packages/ui-kit/src/Breadcrumbs/BreadcrumbItem.tsx (1)
1-41: Component Structure Looks Good!The
BreadcrumbItemcomponent is well-structured and uses React.memo for performance optimization. The use of props and CSS modules is appropriate.Tools
Biome
[error] 28-28: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
packages/ui-kit/src/Breadcrumbs/Breadcrumbs.fixture.tsx (2)
32-39: LGTM! Fixture setup is clear and functional.The fixture correctly sets up the Breadcrumbs component with state management and styling for demonstration purposes.
1-2: Ensure correct import paths.The import paths for
useStateandBreadcrumbsappear correct, assuming theBreadcrumbscomponent is located in the same directory. Verify the paths if there are issues during integration.Verification successful
Import paths are correct.
The import path for the
Breadcrumbscomponent is correct, as theindex.tsxfile is present in the expected directory. No issues with the import statement inBreadcrumbs.fixture.tsx.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the Breadcrumbs component. # Test: Check if the Breadcrumbs component exists in the expected directory. fd 'index.tsx' packages/ui-kit/src/BreadcrumbsLength of output: 88
packages/ui-kit/src/Breadcrumbs/index.tsx (2)
107-123: Ensure accessibility with ARIA labels.The component uses
aria-labelfor navigation, which is good for accessibility. Ensure all interactive elements are keyboard navigable and screen reader friendly.
1-1: Review unused imports.The
useStateanduseCallbackhooks are used, but ensure all other hooks are necessary for the component's functionality.Verification successful
All imported hooks are used in the component.
The
useState,useRef,useEffect, anduseCallbackhooks are all utilized in theBreadcrumbscomponent, confirming there are no unused imports.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of imported hooks in the Breadcrumbs component. # Test: Search for the usage of useRef, useEffect, and useCallback in the file. rg 'useRef|useEffect|useCallback' packages/ui-kit/src/Breadcrumbs/index.tsxLength of output: 270
cosmos/cosmos.userdeps.js (2)
70-70: Ensure correct export mapping for Breadcrumbs.The export mapping for the Breadcrumbs fixture seems correct. Verify that the fixture is correctly loaded and used in the application.
18-18: Verify the new fixture import path.Ensure that the path for the new Breadcrumbs fixture is correct and that the file exists.
Verification successful
Import path verified successfully.
The import path for the Breadcrumbs fixture is correct, and the file exists at the specified location. No issues found with the import statement.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the Breadcrumbs fixture file. # Test: Check if the Breadcrumbs fixture file exists in the expected directory. fd 'Breadcrumbs.fixture.tsx' packages/ui-kit/src/BreadcrumbsLength of output: 116
| }: BreadcrumbItemProps) => { | ||
| return ( | ||
| <li className={cn(styles.breadcrumbItem)}> | ||
| <button className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}> |
There was a problem hiding this comment.
Specify Button Type to Prevent Unintended Behavior
Add an explicit type="button" to the button element to prevent unintended form submissions if this component is used within a form.
- <button className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}>
+ <button type="button" className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(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.
| <button className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}> | |
| <button type="button" className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}> |
Tools
Biome
[error] 28-28: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| const BreadcrumbsContainer = () => { | ||
| const [activeCrumb, setActiveCrumb] = useState<string | null>(null); | ||
|
|
||
| const handleClick = (value: string) => { | ||
| setActiveCrumb(value); | ||
| }; |
There was a problem hiding this comment.
Consider initializing activeCrumb with a default value.
Initializing activeCrumb with a default value could improve the initial state handling, especially if a default breadcrumb is intended to be active.
- const [activeCrumb, setActiveCrumb] = useState<string | null>(null);
+ const [activeCrumb, setActiveCrumb] = useState<string | null>('product1_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 BreadcrumbsContainer = () => { | |
| const [activeCrumb, setActiveCrumb] = useState<string | null>(null); | |
| const handleClick = (value: string) => { | |
| setActiveCrumb(value); | |
| }; | |
| const BreadcrumbsContainer = () => { | |
| const [activeCrumb, setActiveCrumb] = useState<string | null>('product1_0'); | |
| const handleClick = (value: string) => { | |
| setActiveCrumb(value); | |
| }; |
| const ellipsis = '...'; | ||
| const ellipsisWidth = 35; // Approximate width of the `... >` element |
There was a problem hiding this comment.
Consider dynamic ellipsis width calculation.
The ellipsisWidth is hardcoded. Consider dynamically calculating it based on the actual rendered width for better accuracy.
- const ellipsisWidth = 35; // Approximate width of the `... >` element
+ const ellipsisWidth = measureEllipsisWidth(); // Function to measure actual widthCommittable suggestion was skipped due to low confidence.
| const handleItemClick = useCallback( | ||
| (value: string) => { | ||
| if (value !== ellipsis) { | ||
| onClick && onClick(value); | ||
| } | ||
| }, | ||
| [onClick], | ||
| ); |
There was a problem hiding this comment.
Use optional chaining for safety.
Refactor to use optional chaining for the onClick callback to prevent potential runtime errors.
- onClick && onClick(value);
+ onClick?.(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.
| const handleItemClick = useCallback( | |
| (value: string) => { | |
| if (value !== ellipsis) { | |
| onClick && onClick(value); | |
| } | |
| }, | |
| [onClick], | |
| ); | |
| const handleItemClick = useCallback( | |
| (value: string) => { | |
| if (value !== ellipsis) { | |
| onClick?.(value); | |
| } | |
| }, | |
| [onClick], | |
| ); |
Tools
Biome
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| useEffect(() => { | ||
| if (!olRef.current) return; | ||
| const widths = Array.from(olRef.current.children).map((child) => (child as HTMLElement).offsetWidth); | ||
| setItemWidths(widths); | ||
| }, [items]); |
There was a problem hiding this comment.
Optimize width calculation with useLayoutEffect.
Consider using useLayoutEffect instead of useEffect for DOM measurement to avoid layout thrashing.
- useEffect(() => {
+ useLayoutEffect(() => {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.
| useEffect(() => { | |
| if (!olRef.current) return; | |
| const widths = Array.from(olRef.current.children).map((child) => (child as HTMLElement).offsetWidth); | |
| setItemWidths(widths); | |
| }, [items]); | |
| useLayoutEffect(() => { | |
| if (!olRef.current) return; | |
| const widths = Array.from(olRef.current.children).map((child) => (child as HTMLElement).offsetWidth); | |
| setItemWidths(widths); | |
| }, [items]); |
| border-radius: var(--border-radius); | ||
| color: var(--text-faint-strong-up); | ||
|
|
||
| &:focus { |
There was a problem hiding this comment.
It's important to show focus outlines from keyboard focus event only, as described in the task.
Here is an example of how to achieve such behavior: https://css-tricks.com/keyboard-only-focus-styles/
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Files selected for processing (9)
- packages/ui-kit/src/Breadcrumbs/Breadcrumbs.fixture.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/components/BreadcrumbItem/BreadcrumbItem.module.css (1 hunks)
- packages/ui-kit/src/Breadcrumbs/components/BreadcrumbItem/BreadcrumbItem.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/components/Ellipsis/Ellipsis.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/components/commonStyles.module.css (1 hunks)
- packages/ui-kit/src/Breadcrumbs/components/index.ts (1 hunks)
- packages/ui-kit/src/Breadcrumbs/hooks/useModelHook.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/index.tsx (1 hunks)
- packages/ui-kit/src/Breadcrumbs/style.module.css (1 hunks)
Additional context used
Biome
packages/ui-kit/src/Breadcrumbs/components/BreadcrumbItem/BreadcrumbItem.tsx
[error] 24-24: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
packages/ui-kit/src/Breadcrumbs/hooks/useModelHook.tsx
[error] 27-27: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.(lint/style/useNumberNamespace)
[error] 28-28: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.(lint/style/useNumberNamespace)
Additional comments not posted (13)
packages/ui-kit/src/Breadcrumbs/components/index.ts (1)
1-2: Exports are correctly set up.The export statements are straightforward and correctly set up for the
BreadcrumbItemandEllipsiscomponents.packages/ui-kit/src/Breadcrumbs/components/commonStyles.module.css (1)
1-5: Styles for.separatorlook good.The styles are appropriately set for the
.separatorclass, ensuring proper margin and color.packages/ui-kit/src/Breadcrumbs/style.module.css (1)
1-10: Styles for.breadcrumbsare well-defined.The styles ensure a consistent and visually appealing layout for the breadcrumbs component, including background color, shadow, and padding.
packages/ui-kit/src/Breadcrumbs/components/BreadcrumbItem/BreadcrumbItem.module.css (3)
1-6: Styles for.breadcrumbItemare appropriate.The styles define a flexible layout for breadcrumb items, ensuring proper alignment and size.
8-24: Button styles are comprehensive and accessible.The styles for the
.buttonclass include focus-visible and hover states, enhancing accessibility and user interaction.
26-28: Styles for.breadcrumbLabelare concise.The
text-wrap: nowrap;ensures that the label text does not wrap, maintaining a clean appearance.packages/ui-kit/src/Breadcrumbs/components/BreadcrumbItem/BreadcrumbItem.tsx (3)
1-6: Imports are well-organized.The imports include necessary modules and styles, ensuring the component is properly set up.
8-18: Interface definitions are clear.The
BreadcrumbBaseandBreadcrumbItemPropsinterfaces are well-defined, providing clarity on the expected props.
35-35: Display name is correctly set.Setting the
displayNamefor theBreadcrumbItemcomponent aids in debugging and improves readability.packages/ui-kit/src/Breadcrumbs/Breadcrumbs.fixture.tsx (1)
1-43: LGTM!The code is well-structured and demonstrates a clear example of using the Breadcrumbs component.
packages/ui-kit/src/Breadcrumbs/components/Ellipsis/Ellipsis.tsx (1)
1-48: LGTM!The Ellipsis component is well-implemented and efficiently handles hidden breadcrumb items.
packages/ui-kit/src/Breadcrumbs/index.tsx (1)
1-71: LGTM!The Breadcrumbs component is well-structured, with clear handling of overflow and item rendering.
packages/ui-kit/src/Breadcrumbs/hooks/useModelHook.tsx (1)
1-82: LGTM!The
useModelhook is well-implemented and efficiently manages breadcrumb overflow logic. The use ofparseFloataligns with the codebase's preferences.Tools
Biome
[error] 27-27: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.(lint/style/useNumberNamespace)
[error] 28-28: Use Number.parseFloat instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseFloat instead.(lint/style/useNumberNamespace)
| export const BreadcrumbItem = React.memo( | ||
| ({ label, value, active = false, onClick, separator, isLastItem = false }: BreadcrumbItemProps) => { | ||
| return ( | ||
| <li className={cn(styles.breadcrumbItem)}> | ||
| <button className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}> | ||
| <Text type="caption" className={styles.breadcrumbLabel}> | ||
| {label} | ||
| </Text> | ||
| </button> | ||
| {!isLastItem && <div className={commonStyles.separator}>{separator}</div>} | ||
| </li> | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Add explicit button type to prevent form submission.
The button should have an explicit type to avoid unintended form submissions within forms.
<button className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}>
+ type="button"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 BreadcrumbItem = React.memo( | |
| ({ label, value, active = false, onClick, separator, isLastItem = false }: BreadcrumbItemProps) => { | |
| return ( | |
| <li className={cn(styles.breadcrumbItem)}> | |
| <button className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}> | |
| <Text type="caption" className={styles.breadcrumbLabel}> | |
| {label} | |
| </Text> | |
| </button> | |
| {!isLastItem && <div className={commonStyles.separator}>{separator}</div>} | |
| </li> | |
| ); | |
| }, | |
| ); | |
| export const BreadcrumbItem = React.memo( | |
| ({ label, value, active = false, onClick, separator, isLastItem = false }: BreadcrumbItemProps) => { | |
| return ( | |
| <li className={cn(styles.breadcrumbItem)}> | |
| <button type="button" className={cn(styles.button, { [styles.active]: active })} onClick={() => onClick(value)}> | |
| <Text type="caption" className={styles.breadcrumbLabel}> | |
| {label} | |
| </Text> | |
| </button> | |
| {!isLastItem && <div className={commonStyles.separator}>{separator}</div>} | |
| </li> | |
| ); | |
| }, | |
| ); |
Tools
Biome
[error] 24-24: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a
formelement. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset(lint/a11y/useButtonType)
| }; | ||
|
|
||
| const BreadcrumbsContainer = () => { | ||
| const [activeCrumb, setActiveCrumb] = useState<string | null>(null); |
There was a problem hiding this comment.
Consider initializing activeCrumb with a default value.
Initializing activeCrumb with a default value could improve the initial state handling, especially if a default breadcrumb is intended to be active.
- const [activeCrumb, setActiveCrumb] = useState<string | null>(null);
+ const [activeCrumb, setActiveCrumb] = useState<string | null>('product1_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 [activeCrumb, setActiveCrumb] = useState<string | null>(null); | |
| const [activeCrumb, setActiveCrumb] = useState<string | null>('product1_0'); |
|
|
||
| // TODO: get rid of this hardcode and add ability to dynamically count width | ||
| export const ellipsis = '...'; | ||
| export const ellipsisWidth = 35; // Approximate width of the `... >` element |
There was a problem hiding this comment.
Consider dynamic ellipsis width calculation.
The ellipsisWidth is hardcoded. Consider dynamically calculating it based on the actual rendered width for better accuracy.
- export const ellipsisWidth = 35; // Approximate width of the `... >` element
+ export const ellipsisWidth = measureEllipsisWidth(); // Function to measure actual widthCommittable suggestion was skipped due to low confidence.
| interface BreadcrumbItemProps extends BreadcrumbBase { | ||
| active?: boolean; | ||
| onClick: (value: string) => void; | ||
| separator?: ReactNode; |
There was a problem hiding this comment.
Do we need default separator?
There was a problem hiding this comment.
Do we really need "Hook" postfix in the filename?
| ellipsisWidth: number; | ||
| } | ||
|
|
||
| export const useModel = <T,>({ items, ellipsisWidth }: UseModelProps<T>) => { |
There was a problem hiding this comment.
useModel is a very generic name. I had to spend some time to realize this hook is specific to Breadcrumbs component. Would be nice to rename it to something more specific to make it more obvious
|
|
||
| if (totalWidth <= containerWidth) { | ||
| // No overflow, display all items | ||
| setLeftHiddenItemIndex(-1); |
There was a problem hiding this comment.
Maybe it would be easier and more obvious to check for null, instead of [-1; items.length]?
Plus, this interval kinda breaks the logic of "all items between two indexes need to be hidden"
| import styles from './BreadcrumbItem.module.css'; | ||
| import type { ReactNode } from 'react'; | ||
|
|
||
| export interface BreadcrumbBase { |
There was a problem hiding this comment.
NIT: BreadcrumbBase doesn't sound like a label-value interface name. Maybe something like BreadcrumbItemValue?
|
I've tried to dynamically (on item click) change |
https://kontur.fibery.io/Tasks/Task/Add-map-breadcrumbs-component-to-FE-library-14063
Summary by CodeRabbit
New Features
Breadcrumbscomponent for improved navigation within applications.BreadcrumbItemfor reusable breadcrumb elements.Ellipsiscomponent for managing overflow in breadcrumb navigation.Styling Enhancements
Bug Fixes