UI: New side nav styles, abstractions - #30568
Conversation
…and to CategoriesMenu and subcomponent
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces two new SCSS mixins, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SideNavItem
participant TooltipTruncatedText
User->>SideNavItem: Render navigation item
SideNavItem->>TooltipTruncatedText: Render title with tooltip (position: right)
TooltipTruncatedText-->>User: Displays title with tooltip on hover
sequenceDiagram
participant User
participant CategoriesMenu
participant TooltipTruncatedText
User->>CategoriesMenu: Render categories menu
CategoriesMenu->>TooltipTruncatedText: Render category label with tooltip (position: right)
TooltipTruncatedText-->>User: Displays label with tooltip on hover
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
Caution No docstrings were generated. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/styles/var/mixins.scss (1)
408-411: Consider making sticky positioning configurable.The hard-coded
top: 217pxvalue could become a maintenance burden. Consider implementing this as a mixin parameter or calculating it dynamically when feasible.@mixin side-nav-list($sticky-top: 217px) { // ... top: $sticky-top; // ... }changes/16846-update-side-nav-styles (1)
1-1: Fix typo in changelog entry.There's a typo in "side nave" - it should be "side nav".
-* Update side nave styles across the app +* Update side nav styles across the app
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
changes/16846-update-side-nav-styles(1 hunks)frontend/components/TooltipTruncatedText/TooltipTruncatedText.tsx(3 hunks)frontend/pages/admin/components/SideNav/_styles.scss(1 hunks)frontend/pages/admin/components/SideNavItem/SideNavItem.tsx(2 hunks)frontend/pages/admin/components/SideNavItem/_styles.scss(1 hunks)frontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/CategoriesMenu.tsx(2 hunks)frontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/_styles.scss(1 hunks)frontend/pages/hosts/details/cards/Software/SelfService/_styles.scss(0 hunks)frontend/styles/var/colors.scss(1 hunks)frontend/styles/var/colors.ts(1 hunks)frontend/styles/var/mixins.scss(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/pages/hosts/details/cards/Software/SelfService/_styles.scss
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/CategoriesMenu.tsx (1)
frontend/pages/hosts/details/cards/Software/SelfService/helpers.tsx (1)
ICategory(5-12)
🪛 Biome (1.9.4)
frontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/CategoriesMenu.tsx
[error] 33-37: 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)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
- GitHub Check: build-binaries
- GitHub Check: test-js (ubuntu-latest)
- GitHub Check: lint-js (ubuntu-latest)
- GitHub Check: publish
- GitHub Check: build-and-check
🔇 Additional comments (17)
frontend/styles/var/colors.ts (1)
17-17: LGTM! Color addition follows established conventions.The new
ui-fleet-black-5color with hex value#F4F4F6follows the existing naming pattern and fits appropriately in the gray scale sequence.frontend/components/TooltipTruncatedText/TooltipTruncatedText.tsx (3)
18-18: Good addition with proper typing.The
tooltipPositionprop provides flexibility for tooltip placement while maintaining backward compatibility.
28-28: Excellent default value choice.Setting the default to
"top"maintains backward compatibility for existing usages while enabling new positioning options.
47-47: Proper prop utilization.The
tooltipPositionprop is correctly passed to the ReactTooltip component'splaceattribute.frontend/styles/var/mixins.scss (3)
405-420: Well-implemented mixin with centralized navigation styling.The
side-nav-listmixin effectively centralizes side navigation styling. The sticky positioning and dimensions are appropriate for the current use case.
422-452: Comprehensive item styling with good hover/active states.The
side-nav-itemmixin provides complete styling for navigation items including proper hover states and active styling. The font-weight reset for nested tooltip components is a thoughtful touch.
447-449: Smart handling of nested tooltip font-weight.The font-weight reset for
.__react_component_tooltipwithin active navigation items prevents inheritance issues with tooltip styling.frontend/styles/var/colors.scss (1)
19-19: Perfect consistency between SCSS and TypeScript color definitions.The
$ui-fleet-black-5variable matches the TypeScript definition exactly, maintaining consistency across the codebase.frontend/pages/admin/components/SideNav/_styles.scss (1)
7-7: LGTM! Good refactoring for consistency.The use of the
side-nav-listmixin centralizes navigation styling and improves maintainability across the application.frontend/pages/admin/components/SideNavItem/_styles.scss (1)
2-2: LGTM! Consistent with the refactoring pattern.The use of the
side-nav-itemmixin aligns with the overall refactoring to centralize navigation styling.frontend/pages/admin/components/SideNavItem/SideNavItem.tsx (3)
4-4: Good addition for tooltip functionality.The import of
TooltipTruncatedTextsupports the new tooltip feature for truncated navigation items.
15-17: Improved class application logic.Moving the active class application to the
liwrapper element is semantically correct and aligns with the new HTML structure.
20-24: Excellent semantic HTML improvement and UX enhancement.The changes provide multiple benefits:
- Using
<li>as the wrapper element improves semantic HTML structure- Nesting
<Link>inside<li>follows proper list semantics- Adding
TooltipTruncatedTextwith right-positioned tooltip enhances UX for truncated navigation itemsfrontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/_styles.scss (1)
2-4: LGTM! Consistent mixin usage.The use of both
side-nav-listandside-nav-itemmixins maintains consistency with the navigation styling refactoring across the application.frontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/CategoriesMenu.tsx (3)
5-5: Good addition for tooltip functionality.The import supports the tooltip feature for truncated category labels.
26-26: Excellent semantic HTML improvement.Changing from
<div>to<ul>provides proper semantic markup for the categories list.Also applies to: 56-56
51-51: Great UX enhancement with tooltip integration.The
TooltipTruncatedTextwith right-positioned tooltip improves the user experience for truncated category labels.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30568 +/- ##
==========================================
- Coverage 64.07% 64.07% -0.01%
==========================================
Files 1878 1878
Lines 184482 184441 -41
Branches 5399 5318 -81
==========================================
- Hits 118213 118179 -34
+ Misses 56948 56945 -3
+ Partials 9321 9317 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jacobshandling
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/pages/admin/IntegrationsPage/IntegrationPage.tests.tsx (3)
30-33: Fix typo in comment and consider test approach.The test logic correctly reflects the addition of tooltip functionality, but there's a typo in the comment.
Apply this diff to fix the typo:
- // sidenave label, sidenav tooltip, and card header + // sidenav label, sidenav tooltip, and card headerThe updated assertion count (3 occurrences) correctly accounts for the new tooltip functionality where the text appears in both the sidenav label and tooltip, plus the card header.
72-73: Fix typo in comment.The test logic correctly reflects the tooltip functionality, but there's a typo in the comment.
Apply this diff to fix the typo:
- // sidenave label, sidenav tooltip + // sidenav label, sidenav tooltipThe updated assertion count (2 occurrences) correctly accounts for the text appearing in both the sidenav label and tooltip.
30-33: Consider testing tooltip functionality at the component level.While these test updates correctly reflect the UI changes, they're now testing implementation details (that tooltips duplicate text) rather than user behavior. Consider whether tooltip functionality should be tested in component-specific tests for
SideNavItemorTooltipTruncatedTextrather than in integration tests.This approach makes the integration tests more brittle as they depend on tooltip implementation details. Component-level tests might be more appropriate for verifying tooltip behavior.
Also applies to: 72-73
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/pages/admin/IntegrationsPage/IntegrationPage.tests.tsx(3 hunks)frontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/CategoriesMenu.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/pages/hosts/details/cards/Software/SelfService/CategoriesMenu/CategoriesMenu.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript)
- GitHub Check: build-binaries
- GitHub Check: publish
jacobshandling
assigned
RachelElysia and
ghernandez345 and unassigned
gillespi314 and
RachelElysia
There was a problem hiding this comment.
@jacobshandling just had a quick question about the use of SideNav on the My Device page
| width: 232px; | ||
|
|
||
| // Matches styling in DropdownWrapper > options | ||
| @include side-nav-list; |
There was a problem hiding this comment.
Is it possible to use the actual SideNav component in this location instead of just using the styles? Would allow us to keep the styles in one place instead of pulling them away from the component.
There was a problem hiding this comment.
Agree that would be ideal. Implemented this way for speed and since it is an incremental improvement over the current completely decoupled implementation. Can refactor DUP to use the common component now if you feel it's important, otherwise as a further iteration the next time someone is working on this
| "ui-fleet-black-33": "#B3B6C1", | ||
| "ui-fleet-black-25": "#C5C7D1", | ||
| "ui-fleet-black-10": "#E2E4EA", | ||
| "ui-fleet-black-5": "#F4F4F6", |
There was a problem hiding this comment.
@rachaelshaw @jacobshandling notice this is a new colour used in figma but I couldn't find it in the design system. Can we add this to the design system if we'd like to start using it in designs.
4 participants
#16846
Demo
changes/Summary by CodeRabbit
New Features
Style
Chores