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

Dang 123/Main vertical navigation #19

Merged
merged 44 commits into from Jun 2, 2021
Merged

Dang 123/Main vertical navigation #19

merged 44 commits into from Jun 2, 2021

Conversation

mercedesb
Copy link
Contributor

@mercedesb mercedesb commented Jun 1, 2021

JIRA

Description

Create an easily reusable and composable component for main vertical navigation.

Add a custom decorator for route-aware components in Storybook (there should be very few of these). This decorator does not play well with the Docs add-on due to Vue.extend but unfortunately that's the only way to add routing into a Storybook component since there's no other way to grab the Vue instance. We're manually telling the Docs addon what code to render for the source code examples to solve this.

More info on using decorators to add context to stories here.

Navigation Features

  • Full height main navigation with a slot for the child items you want to compose inside of it
  • Optionally animate the main navigation with a "drawer" effect on click so it takes up less screen real estate (default to true, desktop only)
  • Navigation item component that will dynamically render as a routing aware link if it has a to prop or a button with a slot for the child items you want to compose inside of it
  • The navigation item component is optionally collapsible (default to true, desktop and mobile) to toggle if its child items are visible or not
  • Navigation child item component to correctly render nested child items inside parent nav items

Tests

  • The test suite should be green ✅
  • It should look beautiful in Storybook (222px on desktop, full width and not animated on mobile)

Things to note

  • We take advantage of conditional event binding (another helpful link here)in this PR. We don't always want to bind a click event. For example if the nav item is a link and not a button, we want to make sure we don't add a click.
  • We also have nested click events so we want to make sure that we stop propagation so that when we want to toggle the sub nav, we don't also collapse the drawer. Vue has super nice event modifiers that we can take advantage of (and do! 🙂 )
  • Because the nav items need to style themselves based on the state of their parent nav (whether expanded or collapsed for the "drawer" animation), we are making use of scoped slots and slot props.
  • The main navigation styles make use of Tailwind's @screen directive. Because there are some animations and other styles that are not Tailwind-y and will not be reused, they are defined in the component styles themselves. They are also different between desktop and mobile so Tailwind's @screen is super helpful to make sure we're using the breakpoints configured in the project with no variable duplication.

Areas of Concern

  • There is a known issue in Vue (looks like it will be resolved in 2.6.13) that raises a warning about the native click event on dynamic components. There is no way around this currently and it is only a warning, not an error. I quieted the specs but you may see this in the console. Do not be alarmed 🙃

GIFs/Memes

phew

Reviewer Checklist

This section is to be filled out by reviewers

Testing

  • This code was tested by somebody other than the developer. Do not merge until this has been done.

In case there are some scenarios where we don't want to support collapsing
nav (drawer animation) or nav items (toggling sub navs), add a prop that
defaults to true so that we can choose to turn these features off.
Copy link
Contributor

@bethqiang bethqiang left a comment

Choose a reason for hiding this comment

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

Few small things here and there, but this looks so very pretty!

Mercedes and I talked about this privately, but for posterity: the expand/collapse functionality seems unideal to me, in that if the nav is full height, the only place you can really click to expand/collapse is to the left of child items (and how would a user know that?). Will put in our qs doc and flag for Shampa to add to LION's list.

src/components/MainNavigation/MainNavigation.mdx Outdated Show resolved Hide resolved
src/components/MainNavigation/MainNavigation.vue Outdated Show resolved Hide resolved
<template>
<li class="list-none">
<component
:is="tag"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is SO COOL! 🤩

Copy link
Contributor Author

Choose a reason for hiding this comment

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

❤️

@mercedesb mercedesb merged commit fc2a62b into main Jun 2, 2021
@mercedesb mercedesb deleted the DANG-123/main-nav branch June 2, 2021 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants