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

Add active state to top navigation #7933

Merged
merged 2 commits into from
Sep 7, 2018
Merged

Add active state to top navigation #7933

merged 2 commits into from
Sep 7, 2018

Conversation

kkemple
Copy link
Contributor

@kkemple kkemple commented Sep 6, 2018

This PR implements active state for the top navigation of the www site. Currently it just sets the font weight of the active item bold, but any styles can be added to the activeNavItemStyles object in src/components/navigation.js.

I figured @fk would have some input based off the convo in #7425.

Closes #7425

@kkemple kkemple self-assigned this Sep 6, 2018
@fk
Copy link
Contributor

fk commented Sep 6, 2018

Hey Kurt! To quote myself (from my review of the other PR we got for this):

I was sceptic if font-weight: bold would be a good idea, but this is working quite well and I think we should get this in while discussing if we want to go down a different path in terms of styling.

…so still 👍for getting this in as it is right now.

For all of my ongoing .org nav redesign mocks (Figma), I've chosen the "2px underline" variant for now—that's the second option from the ones shown below:

bildschirmfoto 2018-08-20 um 16 54 09

I think any of those will work; the one I currently opted for most probably being the least clear of all options. What do you think?

Any of those free us of the slight jumping connected to font-weight: bold.
The first two options should be the least costly in regards to implementation.

@LekoArts
Copy link
Contributor

LekoArts commented Sep 6, 2018

Any reason why you didn’t use getProps?
https://reach.tech/router/api/Link

@kkemple
Copy link
Contributor Author

kkemple commented Sep 6, 2018

@LekoArts fix for that coming

@kkemple
Copy link
Contributor Author

kkemple commented Sep 7, 2018

@fk, let me play with styles a bit, I think we can prevent jumping when using bold, will also add underline

@kkemple
Copy link
Contributor Author

kkemple commented Sep 7, 2018

here is the outcome of style adjustments, @fk feel free to alter colors etc once merged!

@LekoArts the implementation is now using getProps function

screen shot 2018-09-07 at 10 22 31 am

@fk
Copy link
Contributor

fk commented Sep 7, 2018

Yay! Let's get this in!

I saw a couple of things related to a change of the header height that this PR introduces (e.g. the sidebar in your last screenshot overlapping the bottom border of the header).
I want to offer to take care of myself if you don't mind—as I'm not sure yet how to tackle this, and I don't want to annoy you by making this a long forth and back here. You OK with this? Would take care of this right now.

I don't think this needs any changes to the active style itself btw!

@fk fk merged commit f12689b into master Sep 7, 2018
fk added a commit that referenced this pull request Sep 8, 2018
…(active nav state)) (#7995)

* Use the palette

* Backticks

* Stop guesstimating presets.bannerHeight…

and actually use it. :-)
Also allow any amount of (one-line, scrolling overflow) copy in the banner.

* use presets.bannerHeight to define the height of the banner component instead of deriving presets.bannerHeight from padding and font-size/line-height
* don’t wrap white-space in banner copy and add horizontal overflow scrolling
* slightly reduce banner height

* Bring back fixed header height, fix its position being off

* DRY

* Fix left stop of banner mask-image

* Refactor main navigation and a bunch of related things

* add a couple of constants that should make the header CSS easier to follow
* tidy up here and there, add „SocialNavItem“ component
* bring back the header search’s icon which got lost somewhere along the way
* refine search styles
* properly align all header nav items and search input text so that their text sits on the same baseline as the logo wordmark
* even out horizontal navItem margins
* slightly update navItem hover style
* remove all occurences where we substract 1px (e. g.`calc(100vh - ${presets.headerHeight} - 1px`) but really never needed to
* fix superfluous whitespace above the header on the homepage
@pieh pieh deleted the topics/top-nav-active-state branch November 1, 2018 16:59
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.

[www] doesn't show selected state along top nav bar
3 participants