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

feat: improve display of releases in navbar #906

Merged
merged 9 commits into from
Dec 8, 2021

Conversation

tuananh
Copy link
Contributor

@tuananh tuananh commented Dec 3, 2021

Signed-off-by: Tuan Anh Tran [email protected]

1. Issue, if available:

#826

2. Description of changes:

show the latest version in navbar + style pre-release in gray color

image

3. Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: link to issue
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@netlify
Copy link

netlify bot commented Dec 3, 2021

✔️ Deploy Preview for karpenter-docs-prod ready!

🔨 Explore the source changes: e727612

🔍 Inspect the deploy log: https://app.netlify.com/sites/karpenter-docs-prod/deploys/61aedd5a14b7c40007fea3df

😎 Browse the preview: https://deploy-preview-906--karpenter-docs-prod.netlify.app

@tuananh
Copy link
Contributor Author

tuananh commented Dec 3, 2021

@geoffcline can you take a look?

website/assets/scss/_variables_project.scss Outdated Show resolved Hide resolved
website/config.toml Outdated Show resolved Hide resolved
website/config.toml Outdated Show resolved Hide resolved
@geoffcline geoffcline added documentation Improvements or additions to documentation hacking Related to getting setup and/or working on Karpenter labels Dec 4, 2021
@tuananh
Copy link
Contributor Author

tuananh commented Dec 4, 2021

@geoffcline

  1. i did a hack by patching docsy theme. Lemme see if i can upstream this change so we don't have to keep the patch here. basically, what it does is trying to get the path dir and see if it matches with any of the docs folder and display the {{ .version }} there.
  2. i remove pre-release as well as the gray out css bit. since we dont have pre-release, so its' not needed right?

when browsing in /docs/....
image

when browsing in older docs
image

@tuananh
Copy link
Contributor Author

tuananh commented Dec 4, 2021

i think i need to update netlify.toml too to apply the patch for preview. apparently, it's not using make website command

EDIT: netlify works now

@akestner
Copy link
Contributor

akestner commented Dec 6, 2021

Will this ensure that the docs version dropdown is visible on mobile devices as well?

@tuananh
Copy link
Contributor Author

tuananh commented Dec 7, 2021

Will this ensure that the docs version dropdown is visible on mobile devices as well?

no. the navbar is kind of jammed right now. in order to show version, we may have to hide other links. what do you think we should hide under hamburger menu?

ok now ;)

@tuananh
Copy link
Contributor Author

tuananh commented Dec 7, 2021

Will this ensure that the docs version dropdown is visible on mobile devices as well?

it should show on mobile as well now

image

@rothgar
Copy link
Contributor

rothgar commented Dec 7, 2021

This lgtm. Is the pre-release version added to config.toml manually when we have one to deploy?

I don't see any problem carrying the patch in layouts/partials because that's where you're supposed to modify/extend docsy anyway. It would be nice to be an option upstream but I don't think it should block this PR if it's not there.

@chrisnegus
Copy link
Member

chrisnegus commented Dec 8, 2021

@geoffcline have all your issues been addressed? If so, /lgtm from me.

@geoffcline
Copy link
Contributor

Wonderful, thank you!

@ellistarn ellistarn merged commit 7f2fcda into aws:main Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation hacking Related to getting setup and/or working on Karpenter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants