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

72 main nav #264

Merged
merged 258 commits into from
Mar 6, 2019
Merged

72 main nav #264

merged 258 commits into from
Mar 6, 2019

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented Nov 2, 2018

READY FOR REVIEW

Summary

  • Create Main site navigation component

Needed By (Date)

  • ASAP - Event calendar is already using this

Urgency

  • Event calendar is already using this

Steps to Test

  1. Pull this branch
  2. Run grunt styleguide
  3. Check that the main nav behaves as specified in the Figma document

Affected Projects or Products

  • Decanter

Associated Issues and/or People

yvonnetangsu and others added 10 commits November 2, 2018 00:26
* master:
  reversed order of modular scale type mixin (#260)
  122 hero (#259)
  195 added all Stanford approved fonts (#248)
  change class for logo to match new naming conventions. (#257)
  px to rem (#256)
  219 Modular typography (#250)
  fixup! wip. (#255)
  79 card (#241)
  make color change on hover & focus less abrupt (#253)
@yvonnetangsu yvonnetangsu self-assigned this Nov 2, 2018
@sherakama
Copy link
Member

I've created a ticket so we can go over the .eslintrc file's settings: #308

For the sake of this PR, I am going to format based off what is there today.

@sherakama
Copy link
Member

@yvonnetangsu @JBCSU
HUGE PROPS to the no javascript fallback. I think you have created one of, if not the, most accessible main navigations in higher ED and elsewhere.

<script>
// Do this immediately to minimize FOUC.
NodeList.prototype.forEach = NodeList.prototype.forEach || Array.prototype.forEach;
document.querySelectorAll('nav.su-main-nav').forEach(nav => { nav.classList.remove('no-js'); } );
Copy link
Member

Choose a reason for hiding this comment

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

This gets fired off 5 times in the styleguide. Each time the nav is rendered this code gets introduced. This also collectively picks up a nav each time it runs. I suppose that isn't harmful but does have me itching for an optimization. Good for now.

{% endif %}
{% endspaceless %}
</ul>
{% if "su-main-nav--mobile-search" in modifier_class %}
Copy link
Member

Choose a reason for hiding this comment

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

One thought came to mind with this and how we expect it to interact with the full desktop search. Just two forms? Hide the other on mobile?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's what I was thinking but there might be better solutions. Perhaps we could think about this some more when we build the masthead component? 😃

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

GTG

Thank you for the huge lift. I think this nav exceeds our expectations and is ready to be merged in to the main project.

@yvonnetangsu
Copy link
Member Author

GTG

Thank you for the huge lift. I think this nav exceeds our expectations and is ready to be merged in to the main project.

Thank you for reviewing and the cc fixes! There are minor tweaks I'd like to do (eg, turn this into a mixin so we can pass color parameters to @fancy-hover) but happy to get this merged first 😸

@sherakama
Copy link
Member

Yep, there will be revisions. There always is. This is in a very good place at the moment.

Copy link

@josephgknox josephgknox left a comment

Choose a reason for hiding this comment

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

+1 to all of the wonderful things Shea said here. You two deserve huge props for this work. Really excited to see this get merged in.

@yvonnetangsu
Copy link
Member Author

Awesome!! Thank you to you both for the useful suggestions and reviewing along the way! Now I'm going to squash and merge before you change your mind 😁 😁

@yvonnetangsu yvonnetangsu merged commit 33d92a7 into master Mar 6, 2019
@sherakama sherakama deleted the 72-main-nav branch March 6, 2019 01:00
yvonnetangsu added a commit that referenced this pull request Mar 6, 2019
yvonnetangsu added a commit that referenced this pull request Mar 6, 2019
* master:
  Tweaking modular-spacing mixin to be more user friendly (#309)
  120 Skiplinks (#303)
  72 main nav (#264)
JBCSU added a commit that referenced this pull request Mar 7, 2019
* master:
  Add linear gradient background mixin (#304)
  Tweaking modular-spacing mixin to be more user friendly (#309)
  120 Skiplinks (#303)
  72 main nav (#264)
  Update _input.scss (#301)
  Update README.md (#302)
  298-change "sticky-footer" mixin to target <footer> selector  (#299)
  Set base font size for each breakpoint (#295)
  doc: add period (#294)
  151 global footer tweaks (#293)
  Small bugfix for mobile. (#291)
  282: Fix modular-spacing mixins and variables (#290)

# Conflicts:
#	core/js/decanter.js - moved code into core/js/components/main-nav/main-nav.js
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.

4 participants