Skip to content

Comments

style: Rename sidenav background color tokens for clarity and update AppNav hover/focus states#1632

Merged
elizabetdev merged 6 commits intomainfrom
adjust-sidenav-color-tokens
Jan 21, 2026
Merged

style: Rename sidenav background color tokens for clarity and update AppNav hover/focus states#1632
elizabetdev merged 6 commits intomainfrom
adjust-sidenav-color-tokens

Conversation

@elizabetdev
Copy link
Contributor

@elizabetdev elizabetdev commented Jan 21, 2026

Summary

  • Renames --color-bg-sidenav-link → --color-bg-sidenav-link-active
  • Renames --color-bg-sidenav-link-hover → --color-bg-sidenav-link-active-hover
  • Updates AppNav styles to use the renamed tokens consistently for hover, focus, and active states

Motivation

This change prepares the codebase for multi-theme support. The previous token names (sidenav-link and sidenav-link-hover) were ambiguous about their purpose. The new names (sidenav-link-active and sidenav-link-active-hover) clearly indicate these are for active/interactive states, making it easier to define theme variations and maintain consistency across different themes.

Test plan

  • Verify sidenav links highlight correctly on hover
  • Verify sidenav links highlight correctly on focus (keyboard navigation)
  • Verify active sidenav link maintains correct background color
  • Check both light and dark themes render correctly

@changeset-bot
Copy link

changeset-bot bot commented Jan 21, 2026 •
edited
Loading

🦋 Changeset detected

Latest commit: c5b44d8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Jan 21, 2026 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview, Comment Jan 21, 2026 8:24pm

Request Review

@claude
Copy link

claude bot commented Jan 21, 2026 •
edited
Loading

Code Review

✅ The color token renaming improves semantic clarity for theming.

Critical Issues:

  • ⚠️ Duplicate focus styles: app.scss (lines 42-47) and focus.module.scss define identical focus-visible styles → This creates specificity conflicts. Either use Mantine's focusClassName for all components OR use global CSS, not both. The global a:focus-visible, button:focus-visible will override Mantine's .focusRing class making the theme config pointless.

Recommendation: Remove lines 42-47 from app.scss and rely solely on Mantine's focusClassName: focusClasses.focusRing for consistent focus management across all Mantine components.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 21, 2026 •
edited
Loading

E2E Test Results

✅ All tests passed • 61 passed • 4 skipped • 763s

Status Count
✅ Passed 61
❌ Failed 0
⚠️ Flaky 0
⏭️ Skipped 4

Tests ran across 4 shards in parallel.

View full report →

@brandon-pereira
Copy link
Member

brandon-pereira commented Jan 21, 2026

Screenshot 2026-01-21 at 10 17 17 AM Screenshot 2026-01-21 at 10 17 12 AM

Super nit: I noticed while testing the focus states that not all elements correctly apply the same focus UI - and the create dashboard button has 2 focus elements. Happy to punt this till later-but if its easy can we lock down the focus logic on the sidebar?

@brandon-pereira brandon-pereira self-requested a review January 21, 2026 17:19
@elizabetdev
style: Enhance focus styles and add outline focus color to theme
31063fa 
- Introduced a new focus module for consistent focus ring styling.
- Updated theme to include focus class name and outline focus color.
- Modified semantic colors to include 'color-outline-focus'.
- Adjusted app styles to apply outline focus on focus-visible elements.
@elizabetdev
refactor(AppNav): simplify NewDashboardButton component structure
91f5912 
- Changed the structure of the NewDashboardButton to use the Button component as a wrapper for the Link, improving readability and maintainability.
- Removed the separate Link component to streamline the code.
brandon-pereira
brandon-pereira approved these changes Jan 21, 2026
View reviewed changes
Copy link
Member

@brandon-pereira brandon-pereira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@elizabetdev elizabetdev merged commit 5ba7fe0 into main Jan 21, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandon-pereira brandon-pereira brandon-pereira approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@elizabetdev @brandon-pereira