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

[Feature] main site navigation #11501

Merged
merged 83 commits into from
Oct 31, 2024
Merged

[Feature] main site navigation #11501

merged 83 commits into from
Oct 31, 2024

Conversation

yonikid15
Copy link
Contributor

πŸ€– Resolves #10793

πŸ‘‹ Introduction

  • Create new nav menu for entire site
  • Replaces admin side menu

πŸ•΅οΈ Details

  • Ran into a bug after cleaning up my code, so I pushed up what I had before that
  • I noticed that the role switcher is going to need some tweaking. Right now, it takes a few renders before the current nav role updates (which updates the nav menu view).
  • I feel like there's a some styling things I missed as well.
  • I need to (possibly) add localized constants for the Role enum for the role sub menu. However, right now the display name for the roles comes from the rolePermission.php so maybe we can alter that.

πŸ§ͺ Testing

  1. Play around with nav menu

Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

This is really starting to look great. 🀩 I've done a quick readthrough and played with it a bit. Here are some observations.

apps/web/src/components/Layout/AdminLayout/AdminLayout.tsx Outdated Show resolved Hide resolved
api/lang/en/role.php Outdated Show resolved Hide resolved
apps/web/src/components/Layout/Layout.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

I just did another code readthrough and flagged a few more things.

api/app/Enums/Role.php Outdated Show resolved Hide resolved
apps/web/src/components/NavMenu/MainNavMenu.tsx Outdated Show resolved Hide resolved
apps/web/src/components/NavMenu/MainNavMenu.tsx Outdated Show resolved Hide resolved
apps/web/src/components/NavMenu/MainNavMenu.tsx Outdated Show resolved Hide resolved
apps/web/src/components/NavMenu/navlinks.tsx Outdated Show resolved Hide resolved
apps/web/src/components/Router.tsx Outdated Show resolved Hide resolved
apps/web/src/components/Router.tsx Outdated Show resolved Hide resolved
packages/ui/src/components/NavMenu/NavMenu.tsx Outdated Show resolved Hide resolved
packages/ui/src/components/NavMenu/NavMenuWrapper.tsx Outdated Show resolved Hide resolved
@yonikid15
Copy link
Contributor Author

yonikid15 commented Oct 28, 2024

Josh Design Review:
@JoshBeveridge can you take another look! Should be good to go, other than a couple things that could be spun off into a new issue, so that we can get this merged.

  • Image 1 - Contrast on the hover here definitely doesn't pass. Maybe force it to secondary.darkest on dark mode?
  • Image 2 - Things don't quite seem center aligned to me - I think it's the divider line in particular?
  • Can we make the dividers in the nav base:all(black.light)?
  • Image 3 - The chevron looks different... is it because we're using the mini Heroicon?

It's using the chevron we use site wide (e.g. Accordions). If you want to change the size, then well need to make a new issue.

  • Image 4 - Dropdown padding is a bit off
  • Image 5 - Hover on the notification icon is weird and also contrast again
  • Image 6 - This width isn't ideal on admin, but assuming we can't really fix it right now, can we at least vertically align everything to the middle and have the dividers grow in a similar way to the one next to the notification bell?
  • Image 7 - Can we ensure the mode switcher is centered with the menu? This wasn't in the design because I had all three elements in this section centered together rather than split apart, but I prefer the split you have, so centering makes more sense
  • Image 8 - Right now you can open the notification panel on top of the menu, I think opening either should close the other if its open
  • Image 9 - The page background for the notification pane is different from the menu/dialogs - we should make it the same (use the menu/dialog one)

This one seems a bit difficult right now, is it okay if we make an issue to resolve this later?

  • Image 10 - Can't remember if I mentioned this in my initial review, but the color of these buttons makes me nervous in dark mode... Maybe try inverting them to white?
  • Image 11 - Can we adjust the design of the mobile menu card slightly? Ditch the external border and ensure the internal ones are base(black.darkest.2) base:dark(black.darkest.5)
  • Image 12 - Contrast on some? of the menu items isn't swapping to the light value in dark mode
  • I still can't seem to open submenus with the arrow keys :thinking_face:

Were using the radix navigation menu , which I believe should be accessible out the box. Seems like enter is used to open it, and the arrow keys are used to navigate within the submenu.

Copy link
Member

@tristan-orourke tristan-orourke left a comment

Choose a reason for hiding this comment

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

Alright, that's everything for me! πŸ‘

@petertgiles
Copy link
Contributor

@yonikid15 Could you please check how the new department pages with the new hero look with the new nav? Could you check if you need to add the extra x3 of top padding back in that I removed in this commit?

@yonikid15
Copy link
Contributor Author

@yonikid15 Could you please check how the new department pages with the new hero look with the new nav? Could you check if you need to add the extra x3 of top padding back in that I removed in this commit?

image

Looks good!

@petertgiles
Copy link
Contributor

Wow, this looks incredible! I can't wait to get it merged in. The TOC menus look weird in Chromatic but they are behaving fine in the app so I won't worry about it. 🀷 I noticed two small things. The role menu seems to be sitting a few pixels higher than the rest of the menu items and the contrast is low on the hover effect on the buttons in light mode. The hover looks great in dark mode.
image
image

@yonikid15
Copy link
Contributor Author

yonikid15 commented Oct 31, 2024

@petertgiles The color contrast passes the AA standards on contrast checker so we should be good on that!
Nice catch on the role sub menu vertical alignment πŸ¦… πŸ‘οΈ. Should be fixed in 18e3777

Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

Incredible! It turned out so amazing.

@yonikid15 yonikid15 added this pull request to the merge queue Oct 31, 2024
Merged via the queue into main with commit b9911a5 Oct 31, 2024
12 checks passed
@yonikid15 yonikid15 deleted the 10793-main-site-navigation branch October 31, 2024 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Update the main site navigation
4 participants