Conversation
ogzhanolguncu
requested review from
chronark,
mcstepp and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces two new components to the dashboard: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
apps/dashboard/components/navbar.tsx (1)
85-85: Use Block Statements inifConditions for ConsistencyFor better readability and to follow standard coding practices, it's recommended to wrap the
returnstatement inside a block{}after theifcondition.Apply this change:
- if (!React.isValidElement(child)) return null; + if (!React.isValidElement(child)) { + return null; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 85-85: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a
JsBlockStatement(lint/style/useBlockStatements)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/dashboard/components/navbar.tsx(1 hunks)apps/dashboard/components/page-content.tsx(1 hunks)apps/dashboard/tailwind.config.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/dashboard/components/page-content.tsx
[error] 1-1: All these imports are only used as types.
Importing the types with import type ensures that they are removed by the compilers and avoids loading unnecessary modules.
Safe fix: Use import type.
(lint/style/useImportType)
apps/dashboard/components/navbar.tsx
[error] 85-85: Block statements are preferred in this position.
Unsafe fix: Wrap the statement with a JsBlockStatement
(lint/style/useBlockStatements)
[error] 90-90: Avoid using the index of an array as key property in an element.
This is the source of the key value.
The order of the items may change, and this also affects performances and component state.
Check the React documentation.
(lint/suspicious/noArrayIndexKey)
[error] 93-93: Avoid using the index of an array as key property in an element.
This is the source of the key value.
The order of the items may change, and this also affects performances and component state.
Check the React documentation.
(lint/suspicious/noArrayIndexKey)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apps/dashboard/components/navbar.tsx (4)
35-36: Address background color inconsistencyThe TODO comment indicates a design system inconsistency with the background color. This should be resolved to maintain consistency across the application.
Consider:
- Updating the design system to use the correct background color
- Creating a new design token for this specific use case
- Documenting the decision in the design system documentation
46-52: Add ARIA role for better accessibilityThe Actions component should have an appropriate ARIA role to improve accessibility.
<div ref={ref} + role="toolbar" + aria-label="Page actions" className={cn("flex items-center gap-3", className)} {...props} >
86-110: Enhance keyboard navigation supportConsider adding keyboard navigation support for better accessibility.
<a ref={ref} href={href} + tabIndex={0} + role="link" className={cn( "text-sm transition-colors", active ? "text-accent-12" : "text-accent-10 hover:text-accent-11", dynamic && "font-mono", className, )} {...(active || isLast ? { "aria-current": "page" } : {})} {...props} >
112-123: Consider making ellipsis interactiveThe ellipsis could be enhanced to show hidden breadcrumbs when clicked, improving navigation for users.
Consider implementing a dropdown or popover to display the hidden navigation items when the ellipsis is clicked.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/dashboard/components/navbar.tsx(1 hunks)
🔇 Additional comments (2)
apps/dashboard/components/navbar.tsx (2)
77-78: Consider alternative to index as key
While the current usage of index as key is marked as acceptable, consider if there's a way to derive a more stable key from the child props or content.
#!/bin/bash
# Check if child elements typically have stable props that could be used as keys
ast-grep --pattern 'React.cloneElement($child, { key: $_ })'127-131: LGTM! Well-structured component naming
The display names are properly set and follow a consistent naming convention, which will help with debugging and component identification in React DevTools.
MichaelUnkey
left a comment
MichaelUnkey
left a comment
There was a problem hiding this comment.
Might be a good idea to add this component to our internal UI and Engineering docs
internal/ui/src/components
apps/engineering/content/design/components
Markdown with examples of usage.
Not sure if this or another PR would be better.
This would also give us a place to view the component as @chronark suggested in slack Here.
|
@MichaelUnkey how would you have approached this differently? |
I'm merging this since this is a blocker for breadcrumb clean-up, but let's keep the discussion going. |
|
From Oz ‣ @chronark could you leave an approve so we move on with cleanup |
|
From Oz ‣ yeah on it |
|
From Oz ‣ finding icons on nucleo is hard as f |
|
From Oz ‣ 😄 |
|
did you log in? |
|
From Oz ‣ yeah problem is finding the matching on from figma |
|
From Oz ‣ I think i'll tell Vitor to update names on Figma like This svg is search-input |
|
no need to ask vitor, he already helped |
@MichaelUnkey |
3 participants
What does this PR do?
ENG-1585
Example Usage:
This PR also contains another small component that will be required for the Navbar. Currently, App relies heavily on Layout and adds padding through it. However, this approach creates issues since we can't add padding individually to each page. To fix this problem, we'll use:
We'll wrap our pages with this component, which will handle padding for us. This will allow us to freely adjust padding for the navbar and other components.
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
This is how it will look.
Summary by CodeRabbit
New Features
Navbarcomponent with navigation functionality, includingActions,Breadcrumbs, andLinkcomponents.PageContentcomponent for consistent layout management with responsive padding.Documentation