-
Notifications
You must be signed in to change notification settings - Fork 40
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
Match chrome (header and sidebar nav) to new docs #790
Conversation
|
🟢 No design token changes found |
|
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.
Hey there! This looks awesome! Leaving some feedback from the deployment link.
![Screenshot 2024-10-21 at 12 05 34 PM](https://private-user-images.githubusercontent.com/586552/378508790-56ae5654-c723-4990-beb2-b14b784c369e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk5NTQyNjgsIm5iZiI6MTczOTk1Mzk2OCwicGF0aCI6Ii81ODY1NTIvMzc4NTA4NzkwLTU2YWU1NjU0LWM3MjMtNDk5MC1iZWIyLWIxNGI3ODRjMzY5ZS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxOVQwODMyNDhaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1hZjJlNzg0ODdiOTZjMjM3M2Q0OWQxNTlkZjQ5ZDYwZjA2N2ExZjkwNTE2MDM4NWU0NzA5ZThlYmE2ZDQ5M2RjJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.qzmCElAnt2LdImzzyHjHOM_R08QxjFlMP42jgYOElus)
I believe this should be Primer / Brand UI
, since that's the page the user is currently on, and Brand UI should not be listed to the right with the other links.
Is it possible to update the side nav in this PR to match new docs as well? Main things missing is Mona Sans. If the section headers aren't clickable, I think they're fine left in gray/muted color.
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.
This looks great! I'm sorry, I mentioned in Slack but I should have put it here -
![]() ![]() |
… into mp/match-header-to-new-docs
export function Navigation() { | ||
const activeItem = navItems.find((item) => item.label === 'Brand UI') | ||
const otherItems = navItems.filter((item) => item.label !== 'Brand UI') | ||
const sortedNavItems = activeItem ? [activeItem, ...otherItems] : otherItems |
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.
Forgot to change this when we changed the design of the top nav. Pushing up a fix.
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.
Agreed - @mperrotti and I actually spoke about this last night. Will fix before it gets merged.
Co-authored-by: Rez <[email protected]>
Co-authored-by: Rez <[email protected]>
… into mp/match-header-to-new-docs
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.
Approving assuming we can fix the suggestions mentioned. Thank you!!
Left a small comment here: #790 (comment)
@emilybrick - that's an easy change to bring into here. I'll add that and then merge :) I think I just styled with CSS variables that don't exist in Primer Brand. |
… into mp/match-header-to-new-docs
@rezrah - any idea what could be causing this NPM error in CI?
|
They're unrelated to your changes @mperrotti - feel free to merge your PR when ready. Several integration tests started failing at the same time for different reasons. I'll take a look at fixing those later this week. |
Closes https://github.com/github/primer/issues/4166
We should wait until the new docs site is launched before merging this. Update 02 Dec 2024: soft launch is happening sometime this week - likely Friday 06 Dec 2024.
Summary
Updates the design of the docs site navigation to match the new Primer docs site (https://github.com/github/primer-docs) to give a more "seamless" feel when navigating between the different sub-sites.
We chose not to make these changes in doctocat to avoid accidentally breaking things downstream.
List of notable changes:
What should reviewers focus on?
Confirm that this matches the design of the new Primer docs site.
Steps to test:
Supporting resources (related issues, external links, etc):
Contributor checklist:
Reviewer checklist:
Screenshots:
Note: these have become outdated as we've iterated, but they're pretty close
Child pages
Landing page