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

[Doc] Add navbar machinery #3

Conversation

peytondmurray
Copy link
Owner

@peytondmurray peytondmurray commented Nov 14, 2023

Why are these changes needed?

This PR adds the machinery needed to build the top navigation bar.

The .html files are templates, and they are specified in the appropriate place in conf.py for the theme to pick them up.

The links in the top navigation bar can be specified in navbar.yml, which has a simple syntax that can be easily modified in the future. Here's how this works:

  1. In conf.py, the contents of navbar.yml are parsed and added as a dict to the build context when the config-inited event is emitted.
  2. The render_header_nav_links function is inserted into the build context before the html is rendered, when the html-page-context event is emitted.
  3. The builder calls render_header_nav_links, which actually generates the HTML for the links in the nav bar for each page. This uses the contents of navbar.yml that were inserted into the build context previously to dynamically generate links to various docs pages.

Note that this doesn't need any scripts to run to render the top navigation bar like Ray currently does. The HTML needed is generated during the build step, and makes use of Sphinx's templating system to make this work. The benefits of this approach:

  • The top nav bar renders immediately when the page is loaded. This is in contrast to the current approach, where the navbar is rendered whenever the browser gets around to executing the top-navigation.js script, avoiding a rendering update on page load. This script will be removed in a different PR.
  • Page load times should improve since we aren't executing a script for this job

Related issue number

Partially addresses ray-project#37944.

Depends on #2.

PR 2/x targeting ray-project#41115.

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
    • I've added any new APIs to the API Reference. For example, if I added a
      method in Tune, I've added it in doc/source/tune/api/ under the
      corresponding .rst file.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

Copy link

@maxpumperla maxpumperla left a comment

Choose a reason for hiding this comment

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

LGTM, although it's not clear from this one how exactly the navbar.yml gets picked up. All else looks very standard.

@peytondmurray
Copy link
Owner Author

LGTM, although it's not clear from this one how exactly the navbar.yml gets picked up. All else looks very standard.

It gets picked up here where it is added to the config upon the config-inited event is triggered.

@peytondmurray peytondmurray merged this pull request into docs-build-system-upgrade Nov 16, 2023
1 check passed
@peytondmurray peytondmurray deleted the docs-build-system-upgrade-navbar-machinery branch November 16, 2023 18:50
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.

2 participants