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

Secondary Nav Component - DEC-71 #528

Merged
merged 19 commits into from
Oct 30, 2019
Merged

Secondary Nav Component - DEC-71 #528

merged 19 commits into from
Oct 30, 2019

Conversation

sherakama
Copy link
Member

@sherakama sherakama commented Oct 28, 2019

READY FOR REVIEW

Summary

  • Secondary Nav Components
  • Three options and two colors

Review By (Date)

  • As soon as possible

Urgency

  • Extremely high

Steps to Test

  1. Review tugboat preview for aesthetics (on all breakpoints)
  2. Review code for approach and semantics
  3. Review keyboard controls for both accordion and buttons

Affected Projects or Products

  • D8CORE

Associated Issues and/or People

See Also

@sherakama sherakama temporarily deployed to Tugboat October 28, 2019 21:05 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 28, 2019 21:12 Destroyed
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Looks good! A few things -

  1. I believe we decided at some point to use font-weight: 600 for all level links.
  2. If we follow the Figma design, level 1 font size is 20px, level 2 is 18px and level 3 and deeper is 16px. I'm fine with some variations of this (e.g. 19px, 18px, 17px or 19px, 17px, 16px) but I think it's good to have level 1 largest, level 2 smaller and level 3 and deeper same size and even smaller than level 2.
  3. I'm not checking for current page highlight since that's probably handled by js also.
  4. There's a little tweak to padding-bottom I'd like for the 2nd level link - will post below.

core/src/scss/components/secondary-nav/_secondary-nav.scss Outdated Show resolved Hide resolved
@sherakama sherakama temporarily deployed to Tugboat October 29, 2019 16:10 Destroyed
@sherakama
Copy link
Member Author

sherakama commented Oct 29, 2019

@yvonnetangsu

Your requested changes are up and yes, this is a real branch. I am currently refactoring all the JS to separate it out from the main nav parts. Please review this branch as is. I have sent in a pull request for the Javascript and you can see it at: #529. These two PRs build on top of each other as will the forthcoming refactor for the primary navigation.

@yvonnetangsu
Copy link
Member

@sherakama , the level 3 and 4 font sizes re overridden by level 2 styles so currently they're still 18px -
Screen Shot 2019-10-29 at 11 02 50 AM

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Could we add this bit?

.su-secondary-nav__menu-lv2 .su-secondary-nav__item:last-child .su-secondary-nav__link {
    padding-bottom: 1.4rem;
}

So that there's a bit more padding on the link items that are right above any horizontal dividers?

@yvonnetangsu
Copy link
Member

yvonnetangsu commented Oct 29, 2019

Could we add this bit?

.su-secondary-nav__menu-lv2 .su-secondary-nav__item:last-child .su-secondary-nav__link {
    padding-bottom: 1.4rem;
}

So that there's a bit more padding on the link items that are right above any horizontal dividers?

Before:
Screen Shot 2019-10-29 at 11 25 09 AM

After - more bottom padding under "University Communications", "Twig" and "Tools Launchpad"
Screen Shot 2019-10-29 at 11 24 56 AM

* Javascript Additions.
@sherakama sherakama temporarily deployed to Tugboat October 30, 2019 06:23 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 30, 2019 06:34 Destroyed
@sherakama sherakama temporarily deployed to Tugboat October 30, 2019 06:37 Destroyed
@sherakama sherakama changed the title Secondary Nav Component - NO JS Secondary Nav Component - DEC-71 Oct 30, 2019
@sherakama
Copy link
Member Author

@yvonnetangsu & @kerri-augenstein

Please have a review of the updated tugboat builds. I have completely refactored this secondary navigation to be independent of anything. @yvonnetangsu, there are a number of codeclimate issues and comments that I need to address but please have a look and leave your feedback.

@sherakama sherakama temporarily deployed to Tugboat October 30, 2019 06:47 Destroyed
@yvonnetangsu
Copy link
Member

@kerri-augenstein , in Figma, the mobile menu actually has a max-width and doesn't take on full screen width. I'll post a few questions today just to make sure we're on the same page and using the lingo in the same way 😄
Screen Shot 2019-10-30 at 11 40 11 AM

@kerri-augenstein
Copy link

@yvonnetangsu ohhhhh, I totally forgot about this!!! Wow. Duh. Okay great. So then, I'm thinking maybe we do both??? Make it 400px max-width and kill third and fourth level expand/collapse?

@sherakama
Copy link
Member Author

sherakama commented Oct 30, 2019

@yvonnetangsu @kerri-augenstein

For the sake of this PR let's limit the merge criteria to:

  • Center toggles with link text
  • Limit width to 400px

Refactoring the behavior of the expand/collapse to end after the 3rd level is a big enough effort that I don't want it to block this work.

@yvonnetangsu
Copy link
Member

@yvonnetangsu ohhhhh, I totally forgot about this!!! Wow. Duh. Okay great. So then, I'm thinking maybe we do both??? Make it 400px max-width and kill third and fourth level expand/collapse?

😄 That's a possibility, so I guess my question would be, how does a normally keyboard user navigate a site when they're using a mobile device? I remember Jiatyan did a demo with the Fingate site, where a keyboard user would have to keep tabbing like 100 times to skip through the really long sidebar nav so I just don't want that to happen for those sites with long list of 3rd/4th level links. However, if on mobile, they have a different way to skip through them then it would be fine 🤔

@yvonnetangsu
Copy link
Member

@yvonnetangsu @kerri-augenstein

For the sake of this PR let's limit the merge criteria to:

  • Center toggles with link text
  • Limit width to 400px

Refactoring the behavior of the expand/collapse to end after the 3rd level is a big enough effort that I don't want it to block this work.

Sounds good, @sherakama !

@sherakama sherakama temporarily deployed to Tugboat October 30, 2019 19:14 Destroyed
@sherakama
Copy link
Member Author

@yvonnetangsu @kerri-augenstein

Alignment tightened and width applied. Please review once Tugboat has finished doing its thing.

@sherakama
Copy link
Member Author

Question for @sherakama , do I need to review the main nav also or is that decoupled from this PR now?

The main-nav should be left untouched from the previous version. This PR is specifically just for the sidebar navigation.

Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Will review keyboard behavior in detail after lunch (they mostly look very good so really close).

core/src/scss/components/nav-toggle/_nav-toggle.scss Outdated Show resolved Hide resolved
core/src/scss/components/nav-toggle/_nav-toggle.scss Outdated Show resolved Hide resolved
core/src/scss/components/nav-toggle/_nav-toggle.scss Outdated Show resolved Hide resolved
@sherakama sherakama temporarily deployed to Tugboat October 30, 2019 20:00 Destroyed
@yvonnetangsu
Copy link
Member

Keyboard nav - would be nice when my focus is on a toggle button, if I hit the right arrow, it would open up the submenu for me (similar to the +/- accordion version). So in the below screenshot example, my focus is on the red toggle after "Other Useful Resources", if I hit the right arrow, it should open up the submenu under "Other Useful Resources".

Screen Shot 2019-10-30 at 1 02 11 PM

@sherakama sherakama temporarily deployed to Tugboat October 30, 2019 20:12 Destroyed
Copy link
Member

@yvonnetangsu yvonnetangsu left a comment

Choose a reason for hiding this comment

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

Works great! GTG for the scope of this PR! 🎉

@yvonnetangsu
Copy link
Member

One minor question - I remember you used to have animation when the focus is on the toggle button (the caret gets bigger). Did you decide to remove that?

@sherakama
Copy link
Member Author

One minor question - I remember you used to have animation when the focus is on the toggle button (the caret gets bigger). Did you decide to remove that?

Yes, I am waiting on feedback from Kerri/Ishi on the appropriate motion.

@sherakama sherakama merged commit 5b62e58 into v6 Oct 30, 2019
@sherakama sherakama deleted the secondary-nav branch October 30, 2019 21:11
@sherakama
Copy link
Member Author

THANK YOU SO MUCH FOR ALL THE REVIEW.

^ Caps intentional.

@yvonnetangsu
Copy link
Member

THANK YOU SO MUCH FOR ALL THE REVIEW.

^ Caps intentional.

Thank YOU so much for working on this. This is heroic! 😄

@kerri-augenstein
Copy link

And back to YOU BOTH! An epic amount of effort and love poured into this!!!!!

@yvonnetangsu
Copy link
Member

And back to YOU BOTH! An epic amount of effort and love poured into this!!!!!

Always a pleasure working with you both!!

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.

3 participants