-
Notifications
You must be signed in to change notification settings - Fork 55
Update navigation #603
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
Update navigation #603
Conversation
🦋 Changeset detectedLatest commit: 5170f84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the Canary release to test this locally in primer/design and everything looked as I'd expect. 👍
- Remove monospace font | ||
- Remove dark background and update link colors | ||
- Use UnderlineNav for top right links to provide the correct styling | ||
- Remove "View components" and "React components" from top and leave "Brand" and "About" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilybrick - are we really ready to remove these? I think there are still some React-specific and Rails-specific pages in those sections. Maybe this isn't actually an issue though...when I tested with a local build of primer/design, they were still there because they were still defined in primer/design's primer-nav.yml
(non-blocking feedback)
- Remove monospace font | ||
- Remove dark background and update link colors | ||
- Use UnderlineNav for top right links to provide the correct styling | ||
- Remove "View components" and "React components" from top and leave "Brand" and "About" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a little strange that there's no navigation item for the "main" docs. How do I get back to primer.style/design if I navigate to Brand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point 🤔 . I guess we shouldn't release this changes until this is done? https://github.com/github/primer/issues/2428? @emilybrick
EDIT: Just learned that primer.style redirects to https://primer.style/design
, so the github logo and the Primer design system
title in the top navigation would always bring you back to primer.style/design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to release without the homepage redesign. It just felt strange that the "main" Primer docs don't have their own item in the UnderlineNav.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this if everybody else is
theme/src/components/header.js
Outdated
mr: 2 | ||
}} | ||
> | ||
<UnderlineNav.Link key={index} href={item.url} selected={item.url === path}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
item.url === path
may return false if item.url
is something like "https://primer.style/brand"
, and path
is something like "/brand"
.
I'm not sure if this is actually an issue, but it's something to be aware of when we deploy to prod.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. Should we go with something like this instead?
selected = {item.url === siteMetadata.header.url + path }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try it! Maybe we can test this by opening a draft PR in primer/design so we can test a preview deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preview deployments generate unique domains like https://primer-6f994bf089-26493675.drafts.github.io/
so I don't think using the header.url
will work 😱 . I think there's only one way to know 🚢 🤣
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's do it 🤞
Fixes https://github.com/github/primer/issues/2426
👓 : https://primer-6f994bf089-26493675.drafts.github.io/