Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BED-4810: Vertical Nav #1106

Open
wants to merge 44 commits into
base: stage/v7.0.0
Choose a base branch
from
Open

Conversation

specter-flq
Copy link
Contributor

@specter-flq specter-flq commented Jan 27, 2025

Description

Replaces the top header with a side vertical nav in the left of the app. In this PR we have:

  • The MainNav component which is used in both BHE and CE as well as a SubNav component that is used in the Administration Page of both BHE and CE. Both in bh-shared-ui.
  • We have the tests for each.
  • The Implementation of each in the CE project
  • We have a reorg of routes into a routes folder that has all the routes and the constants that used to be in /ducks/global.

Motivation and Context

This PR addresses: BED-4810

How Has This Been Tested?

Manually and by adding tests.

Screenshots (optional):

Screenshot 2025-01-27 at 4 38 01 PM Screenshot 2025-01-27 at 4 38 26 PM Screenshot 2025-01-27 at 4 39 45 PM Screenshot 2025-01-27 at 4 39 58 PM Screenshot 2025-01-27 at 4 40 10 PM Screenshot 2025-01-27 at 4 40 23 PM

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

@specter-flq specter-flq added the user interface A pull request containing changes affecting the UI code. label Jan 27, 2025
@specter-flq specter-flq self-assigned this Jan 27, 2025
"include": [
"src"
],
"include": ["src", "../../packages/javascript/bh-shared-ui/src/components/AppIcon"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Im curious what the added AppIcon is used for? Was there a problem with local development?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I saw that was added automatically and didn't really question it. I will dig into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like its not needed so i removed it. But will keep an eye on it.

@maffkipp
Copy link
Contributor

Adding a reminder to point this PR to the stage/v7.0.0 branch before merging

@superlinkx superlinkx changed the base branch from main to stage/v7.0.0 January 29, 2025 16:21
@urangel
Copy link
Contributor

urangel commented Jan 29, 2025

The selected nav item background color in dark mode feels like it has too much contrast against the rest of the nav bar. This ends up making the color of the text not have enough contrast when hovered over.

image

Light mode looks okay for this situation.

image

These settings seemed nicer but they might not exactly align with the design system
${isActiveRoute ? 'text-primary dark:text-tertiary bg-neutral-light-4 dark:bg-neutral-dark-5' : 'text-neutral-dark-0 dark:text-neutral-light-1'}

@specter-flq
Copy link
Contributor Author

specter-flq commented Jan 29, 2025

The selected nav item background color in dark mode feels like it has too much contrast against the rest of the nav bar. This ends up making the color of the text not have enough contrast when hovered over.

image

Light mode looks okay for this situation.

image

These settings seemed nicer but they might not exactly align with the design system ${isActiveRoute ? 'text-primary dark:text-tertiary bg-neutral-light-4 dark:bg-neutral-dark-5' : 'text-neutral-dark-0 dark:text-neutral-light-1'}

@urangel Good point, although maybe you need to pull latest because I had already pushed a possible fix for that, I figured to avoid the color contrast issue I would not apply hover color on active but still have the underline to have some feedback. See below and let me know.

Screenshot 2025-01-29 at 12 56 49 PM

cmd/ui/src/App.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@urangel urangel left a comment

Choose a reason for hiding this comment

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

The new nav looks nice! Good work with the expand collapse transitions and thanks for adding the icon creation notes.

@specter-flq
Copy link
Contributor Author

The new nav looks nice! Good work with the expand collapse transitions and thanks for adding the icon creation notes.

Thanks man! but Icon creation notes was the work of @benwaples so kudos to him. I just moved it into bh-shared so we could use it in both.

Copy link
Contributor

@benwaples benwaples left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!!

@specter-flq specter-flq requested a review from maffkipp January 29, 2025 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user interface A pull request containing changes affecting the UI code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants