Component: @phase2/outline-core-breadcrumb
#429
Conversation
|
❌ Deploy Preview for outlinejs failed.
|
WalkthroughThese updates encompass a range of changes aimed at improving functionality, accessibility, and styling across components and documentation. The enhancements include the addition of a pre-commit script for Changes
packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.css: ## Short summary The changes in packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.lightdom.css: This file introduces styles for breadcrumb navigation components, including list styling, flexbox layout, and link decorations. packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.encapsulated.css: The new file packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.global.css: The new file packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.ts: ## Short summary
packages/components/outline-core-breadcrumb/src/resize-controller.ts: This file introduces a packages/documentation/outline-storybook/config/storybook.main.css: ## Short summary The changes in the packages/documentation/outline-storybook/stories/components/outline-core-breadcrumb.stories.mdx: The file packages/components/outline-core-breadcrumb/docs/outline-core-breadcrumb.stories.mdx: This new file introduces a documentation story for the --> 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 Configration File (
|
…utline-core-breadcrumb
…ase2/outline into issue/outline-core-breadcrumb
Co-authored-by: Ofer Shaal <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
package-lock.jsonis excluded by:!**/*.jsonpackage.jsonis excluded by:!**/*.jsonyarn.lockis excluded by:!**/*.lock
Files selected for processing (9)
- .husky/pre-commit (1 hunks)
- README.md (1 hunks)
- packages/components/@deprecated/outline-icon/src/libraries/library.system.ts (1 hunks)
- packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.css (1 hunks)
- packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.lightdom.css (1 hunks)
- packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.ts (2 hunks)
- packages/components/outline-core-breadcrumb/src/resize-controller.ts (1 hunks)
- packages/documentation/outline-storybook/config/storybook.main.css (3 hunks)
- packages/documentation/outline-storybook/stories/components/outline-core-breadcrumb.stories.mdx (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/documentation/outline-storybook/config/storybook.main.css
Additional comments: 13
packages/documentation/outline-storybook/stories/components/outline-core-breadcrumb.stories.mdx (1)
- 6-16: The configuration for hiding the canvas tab in Storybook is correctly implemented. This ensures that users focus on the documentation view for the breadcrumb component, which is likely more relevant for this kind of UI component.
packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.lightdom.css (2)
- 3-18: The use of flexbox for layout in the breadcrumb component is a good choice for responsive design. The styling for
olandulwithlist-style: none;anddisplay: flex;ensures a clean layout without default list styling, which is appropriate for a breadcrumb component.- 31-43: The approach to use graphical elements (
width,height,background,transform) for breadcrumb separators to address accessibility concerns is thoughtful and innovative. This ensures that screen readers do not read out the separator, improving the user experience for visually impaired users.packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.css (2)
- 3-18: The conversion to rem units for margin, padding, and gap properties in the breadcrumb component enhances scalability and maintainability of the styles. This is a good practice for responsive design.
- 31-43: The solution for breadcrumb separators to avoid screen reader issues is consistently applied here as well. This consistency across stylesheets is good for maintainability.
.husky/pre-commit (1)
- 3-5: The check for the
LEFTHOOKenvironment variable allows for an easy bypass of the pre-commit hook if necessary. This can be useful for cases where the hook might be obstructing a commit that needs to be made urgently, though it should be used judiciously.packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.ts (3)
- 1-8: The imports and the use of
AdoptedStyleSheetsfor styling are correctly implemented. This approach allows for more efficient style reuse and encapsulation, which is particularly beneficial for web components.- 30-37: The use of
AdoptedStyleSheetswithinconnectedCallbackis innovative, but ensure that this approach is compatible with all target browsers, asAdoptedStyleSheetsmay not be supported everywhere.- 39-56: The implementation of responsive behavior using
ResizeControllerand conditional rendering based onisMobileis a good practice for creating adaptive components. However, ensure that the logic for determiningisMobileaccurately reflects the intended breakpoints and usage scenarios.packages/components/@deprecated/outline-icon/src/libraries/library.system.ts (1)
- 46-47: Updating the SVG icons for 'home' and 'back' with new paths and dimensions is a good improvement for visual representation. Ensure that these new icons are tested across all platforms where they are used to confirm their appearance and functionality remain consistent.
README.md (1)
- 72-72: Updating the link to the development guide in the README.md file helps ensure that contributors are directed to the most current information. This is a positive change for project documentation.
packages/components/outline-core-breadcrumb/src/resize-controller.ts (2)
- 3-31: The implementation of a debounce function is a common utility that can be very useful in many scenarios, including handling resize events efficiently. This implementation looks correct and should help in reducing the number of resize events handled.
- 42-187: The
ResizeControllerclass is well-structured and provides a robust way to handle element resizing with configurable breakpoints and debouncing. This should significantly enhance the responsiveness of components that use it. Ensure comprehensive testing, especially around the dynamic addition and removal of elements that might affect the resize logic.
| import { html } from 'lit'; | ||
| import { Meta, Canvas, Story } from '@storybook/addon-docs'; | ||
|
|
||
| import { OutlineCoreBreadcrumb } from '../../../../../packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb'; |
There was a problem hiding this comment.
The import path for OutlineCoreBreadcrumb seems overly complex and potentially brittle. Consider simplifying the import path by configuring your build tool to support aliasing or by reorganizing the directory structure.
There was a problem hiding this comment.
This will be updated once this is a package
There was a problem hiding this comment.
@glitchgirl: Understood, it's great to hear that there are plans to simplify the import paths once it becomes a package. If there's any support or assistance needed during this transition, feel free to let me know. Looking forward to seeing the updates!
packages/documentation/outline-storybook/stories/components/outline-core-breadcrumb.stories.mdx
Outdated
Show resolved
Hide resolved
packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.lightdom.css
Outdated
Show resolved
Hide resolved
packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.lightdom.css
Outdated
Show resolved
Hide resolved
packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.css
Outdated
Show resolved
Hide resolved
packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.css
Outdated
Show resolved
Hide resolved
.husky/pre-commit
Outdated
| call_lefthook() | ||
| { | ||
| dir="$(git rev-parse --show-toplevel)" | ||
| osArch=$(uname | tr '[:upper:]' '[:lower:]') | ||
| cpuArch=$(uname -m | sed 's/aarch64/arm64/') | ||
|
|
||
| if lefthook -h >/dev/null 2>&1 | ||
| then | ||
| lefthook "$@" | ||
| elif test -f "$dir/node_modules/lefthook/bin/index.js" | ||
| then | ||
| "$dir/node_modules/lefthook/bin/index.js" "$@" | ||
| elif test -f "$dir/node_modules/@evilmartians/lefthook/bin/lefthook_${osArch}_${cpuArch}/lefthook" | ||
| then | ||
| "$dir/node_modules/@evilmartians/lefthook/bin/lefthook_${osArch}_${cpuArch}/lefthook" "$@" | ||
| elif test -f "$dir/node_modules/@evilmartians/lefthook-installer/bin/lefthook_${osArch}_${cpuArch}/lefthook" | ||
| then | ||
| "$dir/node_modules/@evilmartians/lefthook-installer/bin/lefthook_${osArch}_${cpuArch}/lefthook" "$@" | ||
| elif bundle exec lefthook -h >/dev/null 2>&1 | ||
| then | ||
| bundle exec lefthook "$@" | ||
| elif yarn lefthook -h >/dev/null 2>&1 | ||
| then | ||
| yarn lefthook "$@" | ||
| elif pnpm lefthook -h >/dev/null 2>&1 | ||
| then | ||
| pnpm lefthook "$@" | ||
| elif command -v npx >/dev/null 2>&1 | ||
| then | ||
| npx @evilmartians/lefthook "$@" | ||
| elif swift package plugin lefthook >/dev/null 2>&1 | ||
| then | ||
| swift package --disable-sandbox plugin lefthook "$@" | ||
| else | ||
| echo "Can't find lefthook in PATH" | ||
| fi | ||
| } |
There was a problem hiding this comment.
The script's approach to finding and executing lefthook from various possible locations (global, local node_modules, etc.) is comprehensive. However, consider adding a final else clause to exit with a non-zero status code if lefthook cannot be found, to prevent commits from proceeding without passing the hooks.
There was a problem hiding this comment.
This is for @himerus
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| @@ -0,0 +1,188 @@ | |||
| //THIS IS A TEMP FILE, REPLACE WITH OFER'S PACKAGE WHEN READY | |||
There was a problem hiding this comment.
Acknowledging that this is a temporary file and will be replaced in the future is good for transparency. However, ensure that there is a tracking mechanism (e.g., an issue in the project's issue tracker) to replace this with Ofer's package when ready.
Would you like me to open a GitHub issue to track the replacement of this temporary file with Ofer's package?
…utline-core-breadcrumb
himerus
changed the title
@phase2/outline-core-breadcrumb
himerus
added
Type: Component Suggestion
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (5)
- packages/components/@deprecated/outline-icon/src/libraries/library.system.ts (1 hunks)
- packages/components/outline-core-breadcrumb/docs/outline-core-breadcrumb.stories.mdx (1 hunks)
- packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.encapsulated.css (1 hunks)
- packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.global.css (1 hunks)
- packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- packages/components/@deprecated/outline-icon/src/libraries/library.system.ts
Additional comments: 8
packages/components/outline-core-breadcrumb/docs/outline-core-breadcrumb.stories.mdx (3)
- 1-3: The imports and usage of
Meta,Canvas, andStoryfrom@storybook/addon-docsare correctly implemented, ensuring that the documentation leverages Storybook's features effectively.- 6-16: The
Metaconfiguration is well-defined, setting the title, component, and parameters to enhance the documentation experience. The decision to hide the canvas view by default (hidden: true) focuses the user on the documentation, which is a thoughtful touch for components where code examples and explanations are more critical than visual previews.- 20-37: The breadcrumb example provided uses semantic HTML (
navandulelements) and correctly setsaria-labelandaria-currentattributes, enhancing accessibility. This example serves as a good practice for developers looking to implement the breadcrumb component in their projects.packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.global.css (2)
- 1-16: The CSS for lists and list items is correctly using
flexto create a horizontal layout, which is a common design pattern for breadcrumbs. The use oflist-style: none;on botholandulelements, as well as onlielements, ensures that default list styling does not interfere with the custom breadcrumb design.- 29-41: The approach to creating a breadcrumb separator using CSS properties (
width,height,background,transform) instead of textual content is innovative and addresses accessibility concerns by preventing screen readers from reading the separator as content. This is a commendable practice for improving accessibility.packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.encapsulated.css (1)
- 1-71: The encapsulated CSS for the breadcrumb component is consistent with the global CSS, with adjustments to margins, paddings, and the separator to suit a more compact layout. This demonstrates a thoughtful approach to styling, considering different contexts where the breadcrumb might be used. The use of relative units (
rem) for margins and paddings ensures scalability and consistency across different screen sizes and resolutions.packages/components/outline-core-breadcrumb/src/outline-core-breadcrumb.ts (2)
- 1-9: The imports are correctly organized and include necessary elements from
litand other libraries. The use ofAdoptedStylesheetsfor style encapsulation andResizeControllerfor responsive design demonstrates an advanced understanding of Lit and custom web component development.- 44-57: The updated
rendermethod introduces a responsive design approach by usingResizeControllerto determine if the component is in a mobile view and adjusting the displayed content accordingly. This is a commendable implementation for enhancing user experience across different devices. However, ensure thorough testing across various breakpoints to confirm the responsiveness behaves as expected.
| a { | ||
| text-decoration: none; | ||
| display: inline-flex; | ||
| align-items: center; | ||
|
|
||
| padding: 10px; | ||
| margin: 10px; | ||
|
|
||
| &:hover { | ||
| text-decoration: underline; | ||
| } | ||
| &:focus, | ||
| &:focus-visible { | ||
| outline: solid 3px blue; | ||
| border-radius: var(--border-radius-button-default); | ||
| outline-offset: var(--spacing-px); | ||
|
|
||
| } | ||
| } |
There was a problem hiding this comment.
The styling for links (a elements) includes focus and hover states, which are crucial for accessibility and user experience. The use of outline on focus and text-decoration on hover follows best practices for indicating interactive elements. However, the choice of blue for the outline color might not meet contrast requirements against all backgrounds. Consider using a color that passes WCAG contrast ratio guidelines.
- outline: solid 3px blue;
+ outline: solid 3px var(--outline-focus-color);Ensure --outline-focus-color is defined in a global stylesheet or within this component's styles to meet accessibility contrast requirements.
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.
| a { | |
| text-decoration: none; | |
| display: inline-flex; | |
| align-items: center; | |
| padding: 10px; | |
| margin: 10px; | |
| &:hover { | |
| text-decoration: underline; | |
| } | |
| &:focus, | |
| &:focus-visible { | |
| outline: solid 3px blue; | |
| border-radius: var(--border-radius-button-default); | |
| outline-offset: var(--spacing-px); | |
| } | |
| } | |
| a { | |
| text-decoration: none; | |
| display: inline-flex; | |
| align-items: center; | |
| padding: 10px; | |
| margin: 10px; | |
| &:hover { | |
| text-decoration: underline; | |
| } | |
| &:focus, | |
| &:focus-visible { | |
| outline: solid 3px var(--outline-focus-color); | |
| border-radius: var(--border-radius-button-default); | |
| outline-offset: var(--spacing-px); | |
| } | |
| } |
| static styles: CSSResultGroup = [componentStyles, componentStyles, css``]; | ||
| GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets( | ||
| this, | ||
| globalStyles | ||
| ); | ||
| EncapsulatedStylesheets: AdoptedStylesheets | undefined; | ||
| // THIS NEEDS TO BE HERE FOR STORYBOOK TO WORK | ||
| debug = false; | ||
| resizeController = new ResizeController(this, { | ||
| breakpoints: [768, 1440], | ||
| }); |
There was a problem hiding this comment.
The implementation of GlobalStylesheets and EncapsulatedStylesheets using AdoptedStylesheets is a sophisticated approach to manage styles in a Lit component. However, there's a duplication in the assignment of componentStyles to static styles. This should be corrected to include both global and encapsulated styles.
- static styles: CSSResultGroup = [componentStyles, componentStyles, css``];
+ static styles: CSSResultGroup = [globalStyles, encapsulatedStyles, css``];Ensure that globalStyles and encapsulatedStyles are correctly imported and applied to the component to achieve the intended styling.
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.
| static styles: CSSResultGroup = [componentStyles, componentStyles, css``]; | |
| GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets( | |
| this, | |
| globalStyles | |
| ); | |
| EncapsulatedStylesheets: AdoptedStylesheets | undefined; | |
| // THIS NEEDS TO BE HERE FOR STORYBOOK TO WORK | |
| debug = false; | |
| resizeController = new ResizeController(this, { | |
| breakpoints: [768, 1440], | |
| }); | |
| static styles: CSSResultGroup = [globalStyles, encapsulatedStyles, css``]; | |
| GlobalStylesheets: AdoptedStylesheets | undefined = new AdoptedStylesheets( | |
| this, | |
| globalStyles | |
| ); | |
| EncapsulatedStylesheets: AdoptedStylesheets | undefined; | |
| // THIS NEEDS TO BE HERE FOR STORYBOOK TO WORK | |
| debug = false; | |
| resizeController = new ResizeController(this, { | |
| breakpoints: [768, 1440], | |
| }); |
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist
Summary by CodeRabbit
New Features
ResizeControllerfor dynamic breadcrumb rendering based on screen size.OutlineCoreBreadcrumbcomponent demonstrating its usage.Refactor
Documentation
breadcrumbproperty in the breadcrumb component.