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

Side nav style tweaks #170

Merged
merged 12 commits into from
Jun 18, 2020
Merged

Side nav style tweaks #170

merged 12 commits into from
Jun 18, 2020

Conversation

LizBaker
Copy link
Contributor

@LizBaker LizBaker commented Jun 17, 2020

Description

This PR is for minor style/functionality tweaks in the side nav

pending completion:

  • add link hover
  • on homepage expand first level of nav items, not second

Related Issue(s) / Ticket(s)

Screenshot(s)

Screen Shot 2020-06-17 at 2 44 43 PM

@LizBaker LizBaker added enhancement New feature or request work in progress This is work that is not yet ready for review labels Jun 17, 2020
Copy link
Contributor

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Because these sections can be expanded/collapsed, would it be nice have an animation between the 2 states? If so, we can add an animation that rotates the chevron into the down position. Thoughts?

@LizBaker
Copy link
Contributor Author

yes! that would be much smoother

@LizBaker LizBaker removed the work in progress This is work that is not yet ready for review label Jun 18, 2020
@@ -27,6 +27,11 @@
align-items: center;
justify-content: space-between;
text-decoration: none;
transition: 0.1s;

&:hover {
Copy link
Contributor

@zstix zstix Jun 18, 2020

Choose a reason for hiding this comment

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

Could we maybe add something like this to prevent the hover state for the current page? Super nitpicky, but I think it we should try to communicate that clicking on the current page wont do anything of use.

Suggested change
&:hover {
&:not(.isCurrentPage):hover {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good idea, i'll make some minor tweaks in a new PR so we can get this in 🎉

Copy link
Contributor

@zstix zstix left a comment

Choose a reason for hiding this comment

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

This looks SO GOOD. Thank you for doing this, it really helps!

@LizBaker LizBaker merged commit e4cd2ad into master Jun 18, 2020
@LizBaker LizBaker deleted the liz/sidenav-styles branch June 18, 2020 17:32
@nr-opensource-bot
Copy link
Contributor

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants