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

v3 #218

Merged
merged 16 commits into from
Jul 14, 2022
Merged

v3 #218

merged 16 commits into from
Jul 14, 2022

Conversation

sowasred2012
Copy link
Contributor

@sowasred2012 sowasred2012 commented May 4, 2022

Done

  • Updated global-nav to work specifically with the Vanilla navigation pattern, rather than as a standalone element

QA

  • View the demo
  • On desktop, see that the "All Canonical" nav item is present, and that the dropdown appears when clicked
  • On mobile, see that "All Canonical" is not present, and instead "Products", "Also From Canonical", "Resources" and "About" dropdowns are visible, and work
  • Accessibility: the dropdown should comply with the Vanilla a11y docs: https://vanillaframework.io/docs/patterns/navigation/accessibility

Fixes: https://github.com/canonical-web-and-design/web-squad/issues/4989

@webteam-app
Copy link

Demo starting at https://global-nav-218.demos.haus

@sowasred2012
Copy link
Contributor Author

@davegoddard42 @lyubomir-popov I've assigned you both to this for a review - I'm not sure what we want to do about the mobile view of this, as it is it's what we had in the global nav previously but contained within the navigation element, but it doesn't look particularly "Vanilla", so I'm looking for some guidance on how it should look: https://global-nav-218.demos.haus/

For more context/background, this is a chat I had with @lyubomir-popov and @bartaz about it in Mattermost: https://chat.canonical.com/canonical/pl/5bcdcwperb8tdnbkryi4xhuf3y

@davegoddard42
Copy link

Thanks @sowasred2012. Let me look at this more depth, and we can catch up on Monday @lyubomir-popov. We have a little more time now that the launch has been pushed back a month, so let's get it right.

@lyubomir-popov
Copy link

another alternative would be to have chevrons pointing right and so the nested items as views that slide right to left

@davegoddard42
Copy link

@sowasred2012 apologies, I've not been very attentive to this one. I'm worried about doing too much here as when we launch the mega nav, this drop down will have it's content reviewed and rationalised.

I've set up some metrics to see what the current user interaction is like. Currently few people click on the "Products" drop down in its current state. Let's see what people click on within the dropdown itself - it's on the main page so won't take more than a day or two to get results.

I'm tempted to drop the "Resources" and "About" things and have a list of products only. This would remove the need for nesting.

Other solutions could be to feature the curren top 9 products plus a link to "About" and the engage pages. This would also get rid of the need for nesting.

I'll wait for the metrics to come back, and chat to SEO. If we don't wnat to lose anything from there then we can look at how better to style it.

Long term, this will be overhauled as part of the mega menu anyway, and I don't want to spend lots of time designing something that's soon redundant. Or I want to invest in designing something that has multiple applications. We will need to spend time on the mobile mega menu (I have ideas and rough designs but I'll need Lyubo's help in getting there).

@davegoddard42
Copy link

LGTM @sowasred2012

index.html Outdated Show resolved Hide resolved
import { createNav } from '@canonical/global-nav';
createNav({ maxWidth: '80rem' });
```
3. You will then need to add the `.global-nav` class to a `ul.p-navigation__items` element within the navigation pattern. The module will look for this class and add the dropdown as the first item in the list.
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to optionally be able to use different class name and provide it as a param.

Copy link
Contributor

@anthonydillon anthonydillon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@lyubomir-popov
Copy link

@sowasred2012 all good just one minor cosmetic thing: the space abve the muted heading is way too much.

image

Can you replicae what happens when you have an hr followed by a muted heading?

image

@sowasred2012 sowasred2012 merged commit 9b573d1 into canonical:main Jul 14, 2022
@sowasred2012 sowasred2012 deleted the v3 branch July 14, 2022 14:41
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.

7 participants