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

site: Advanced website navigation #517

Merged
merged 5 commits into from
Feb 28, 2020

Conversation

ahmetb
Copy link
Member

@ahmetb ahmetb commented Feb 27, 2020

Temporary staging at http://staging.krew.dev/

  • added responsive collapsible navbar (+requires bootstrap.js, jquery)
  • added a homepage (previously was docs ToC)
  • added ToC pages for sub-sections (e.g. /docs/user-guide)
  • add 404 page
  • limit the content to 840px to avoid long lines on large screens
  • move logo to /static/img

Signed-off-by: Ahmet Alp Balkan ahmetb@google.com

Related issue: #509 #514

- added responsive collapsible navbar (+requires bootstrap.js, jquery)
- added a homepage (previously was docs ToC)
- added ToC pages for sub-sections (e.g. /docs/user-guide)
- add 404 page
- limit the content to 840px to avoid long lines on large screens

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 27, 2020
Copy link
Contributor

@corneliusweig corneliusweig left a comment

Choose a reason for hiding this comment

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

Looks pretty good.

Two nits:

  • navigation is a bit surprising, because the back button does not go back to the user docs overview page, but the full documentation overview. Maybe nothing that need immediate attention but would be good to solve this at some point.
  • the nav bar is not rendered. Is that intentional?

site/layouts/partials/navbar.html Show resolved Hide resolved
site/layouts/docs/single.html Outdated Show resolved Hide resolved
@ahmetb
Copy link
Member Author

ahmetb commented Feb 27, 2020

  • navigation is a bit surprising, because the back button does not go back to the user docs overview page, but the full documentation overview. Maybe nothing that need immediate attention but would be good to solve this at some point.

YEAHHHH about that.

So normally I was planing to do this:

  • Have a single /docs/ page that shows the Table of Contents
  • ToC only links to leaf pages, sections are unlinked.
  • If user goes to an intermediate docs page by deleting some portion of the URL, e.g. /docs/user-guide/ → 404 Not Found.

Then I pivoted my thinking:

  • Retain the /docs/ home.
  • From / home page have links to User Guide and Developer Guide, by creating ToC pages for those (this actually means /docs/user-guide/setup/ also will have a ToC page, but unless user types it manually, it's unreachable).
  • Problem: User Guide$SOME_PAGEBack takes you to /docs/ home; not User Guide.

Some options we have here:

  1. Don't render sub-section pages e.g. /docs/user-guide/ and just have a huge ToC. (we lose the nice & short User Guide page).

  2. Retain existing (confusing)

  3. Make <-- Back go to "parent" page: Sadly, makes no sense for intermediate pages like Docs > User Guide > SubSection > SubSection2 > Page :(

@corneliusweig
Copy link
Contributor

I think smaller Toc pages are really helpful, so let's try to retain that.

Maybe your option 3 creates the least confusion, even though not all of these intermetidate ToC make a lot of sense?

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@codecov-io
Copy link

codecov-io commented Feb 27, 2020

Codecov Report

Merging #517 into master will decrease coverage by 0.27%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #517      +/-   ##
==========================================
- Coverage   59.12%   58.85%   -0.28%     
==========================================
  Files          23       23              
  Lines        1003     1011       +8     
==========================================
+ Hits          593      595       +2     
- Misses        350      358       +8     
+ Partials       60       58       -2
Impacted Files Coverage Δ
internal/index/indexscanner/scanner.go 66.1% <0%> (-6.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3b34c1...a311da8. Read the comment docs.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
Comment on lines +18 to +19
<script src="https://code.jquery.com/jquery-3.4.1.slim.min.js" integrity="sha384-J6qa4849blE2+poT4WnyKhv5vZF5SrPo0iEjwBvKU7imGFAV0wwj1yYfoRSJoZ+n" crossorigin="anonymous"></script>
<script src="https://stackpath.bootstrapcdn.com/bootstrap/4.4.1/js/bootstrap.min.js" integrity="sha384-wfSDF2E50Y2D1uUdj0O3uMBJnjuUD4Ih7YwaYd1iqfktj0Uod8GCExl3Og8ifwB6" crossorigin="anonymous"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

What are those two good for? I cant see where it's used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Collapsible navbar (first sentence in the PR description) :)
We can use for more stuff in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah.. I should have resized my window 😄

@corneliusweig
Copy link
Contributor

corneliusweig commented Feb 27, 2020

Oh no, there's a broken link: https://staging.krew.dev/docs/developer-guide/release/new-plugin/ -> list item 3 plugin manifest

Nice test for the 404 page though 😉

@ahmetb
Copy link
Member Author

ahmetb commented Feb 27, 2020

Oh no, there's a broken link: staging.krew.dev/docs/developer-guide/release/new-plugin -> list item 3 plugin manifest

fixed.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>
@corneliusweig
Copy link
Contributor

/lgtm
/approve
Nice work! Now we only need to deploy to netlify and we're good :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ahmetb,corneliusweig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit c473daa into kubernetes-sigs:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants