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

Full width dropdowns for Vanilla "meganav" navigation #5213

Merged
merged 17 commits into from
Jul 24, 2024
Merged

Conversation

bartaz
Copy link
Member

@bartaz bartaz commented Jul 10, 2024

Done

This PR adds a new style of top navigation dropdowns that are full-width containers.
Mobile version still uses the same "sliding" navigation lists of items, but desktop dropdowns will contain a separate content, with full support for grid and Vanilla components or any custom HTML.

NOTE: this feature is part of bigger work in progress for upstreaming meganav styling and is only meant to be used in the context of meganav on canonical.com or other websites.

Fixes https://warthogs.atlassian.net/browse/WD-12784

QA

NOTE: the functionality is only about dropdown container, the content of the dropdown is not part of this feature. For demo we have some test content that uses grid.

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix relesase (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

Screenshots

image

@webteam-app
Copy link

@bartaz bartaz force-pushed the meganav-dropdowns branch 2 times, most recently from 6c67176 to c030f3a Compare July 12, 2024 08:43
@bartaz bartaz changed the title WIP: experimenting with meganav dropdowns Full width dropdowns for Vanilla "meganav" navigation Jul 16, 2024
@bartaz bartaz marked this pull request as ready for review July 16, 2024 13:20
@bartaz bartaz added the Review: Percy needed This PR needs a review of Percy for visual regressions label Jul 16, 2024
@pastelcyborg
Copy link
Contributor

pastelcyborg commented Jul 16, 2024

One other comment I'll keep separate from my review, since it seems possibly out of scope - I notice there are some minor wrapping issues at specific screen sizes, particularly while desktop nav is displayed:

image

At the very least, maybe we should keep the Sign In and Search blocks grouped together for wrapping purposes, since the Search icon wrapped by itself looks a bit strange, since it doesn't have a label?

@jmuzina jmuzina added Review: Percy -1 and removed Review: Percy needed This PR needs a review of Percy for visual regressions labels Jul 16, 2024
@bartaz
Copy link
Member Author

bartaz commented Jul 16, 2024

One other comment I'll keep separate from my review, since it seems possibly out of scope - I notice there are some minor wrapping issues at specific screen sizes, particularly while desktop nav is displayed:

Thanks @pastelcyborg. Yes, the wrapping is kind of known issue that would not happen in real use case. We have a $breakpoint-navigation-threshold variable that you adjust based on amount of content in the nav in your project.

This example we have here has a lot more than usual navigations and I don't want to adjust the Vanilla default to accomodate that. The problem is, we can't easly have this setting adjusted per example, because it's part of compile step, so we would need a separate Vanilla build for this specific example.

@pastelcyborg
Copy link
Contributor

One other comment I'll keep separate from my review, since it seems possibly out of scope - I notice there are some minor wrapping issues at specific screen sizes, particularly while desktop nav is displayed:

Thanks @pastelcyborg. Yes, the wrapping is kind of known issue that would not happen in real use case. We have a $breakpoint-navigation-threshold variable that you adjust based on amount of content in the nav in your project.

This example we have here has a lot more than usual navigations and I don't want to adjust the Vanilla default to accomodate that. The problem is, we can't easly have this setting adjusted per example, because it's part of compile step, so we would need a separate Vanilla build for this specific example.

Since the component requires JS anyway, it might be nice to move to a model where we calculate the metrics of the nav discreetly and apply $breakpoint-navigation-threshold automatically, so the user doesn't need to do anything on their end. Definitely out of scope for this PR, though.

@bartaz
Copy link
Member Author

bartaz commented Jul 16, 2024

Since the component requires JS anyway, it might be nice to move to a model where we calculate the metrics of the nav discreetly and apply $breakpoint-navigation-threshold automatically, so the user doesn't need to do anything on their end. Definitely out of scope for this PR, though.

Responsive breakpoints are built on compile-time using the SCSS variables. We can't change that from JS. To move responsiveness to JS we would have to rework how mobile/desktop styles are applied, as it would have to happen not via media queries, but via JS (class name change?). Definitely out of scope .

@bartaz
Copy link
Member Author

bartaz commented Jul 17, 2024

I just realised I messed up something in JS and sliding navigation only works for first level of nested menu, but not for second one. Need to dig out of it.

@bartaz
Copy link
Member Author

bartaz commented Jul 24, 2024

It looks like the 'setFocus' function isn't working and thus keyboard navigation is broken. I addressed this in my PR, here. Basically it doesn't pick up the ul as it is nested

Good catch @petesfrench. I think I managed to address it (it turned out to be a bit more complicated than your fix, but it was a good direction). I also introduced the js-dropdown-nav-list class name that needs to be added to every list that needs to be made focusable/unfocusable.

@petesfrench
Copy link
Contributor

@bartaz Interesting, I have done the same thing. It might be worth doing a review of the code I write and the code in Vanilla to see if we can align them. So you everything can work together out the box as it does on c.com.

@bartaz bartaz merged commit d9a4dd3 into main Jul 24, 2024
14 checks passed
@bartaz
Copy link
Member Author

bartaz commented Jul 24, 2024

Thanks everyone for reviews and tips!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants