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

Add a version indicator and selector to all docs pages #22725

Closed
wants to merge 4 commits into from

Conversation

fmeum
Copy link
Collaborator

@fmeum fmeum commented Jun 12, 2024

The header of each docs page now indicates the current version in bold and contains links to the same page for each other release with versioned docs.

The template language usage in this change is based on publicly available information: https://github.com/GoogleCloudPlatform/DataflowTemplates/blob/97f69c065e9a049f43a5676b633d181ed0ac8448/plugins/core-plugin/src/main/resources/site-template.md?plain=1

Screenshot 2024-06-13 at 12 29 57

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 12, 2024

@meteorcloudy @fweikert This is a wild hack based on guesses on what the templating language looks like, but maybe this can serve as the basis for a proper solution. Is there a way you or I could check whether this at least works, ignoring that it looks horrible for now?

@fmeum fmeum requested a review from meteorcloudy June 12, 2024 17:40
@Wyverald
Copy link
Member

Canonical Compiler Error: <class 'google3.devsite.two.compilers.sweden.templating.exceptions.TemplatingError'>: invalid argument request.path|length in ['request.path|length', '<', '10', 'or', 'request.path|slice(":10")', '!=', "'/versions/'"].

@meteorcloudy
Copy link
Member

Is there a way you or I could check whether this at least works, ignoring that it looks horrible for now?

Unfortunately no, only internal people can build the site.. but we can definitely help with this one, thanks for taking a look!

@fmeum fmeum changed the title WIP: Add an actual version dropdown list to all docs pages Add a version indicator and selector to all docs pages Jun 13, 2024
@fmeum fmeum marked this pull request as ready for review June 13, 2024 10:46
@github-actions github-actions bot added team-Documentation Documentation improvements that cannot be directly linked to other team labels awaiting-review PR is awaiting review from an assigned reviewer labels Jun 13, 2024
@fmeum
Copy link
Collaborator Author

fmeum commented Jun 13, 2024

I updated this to a new approach that doesn't rely on any templating syntax I couldn't find online. I also added a screenshot of the (manually created) result, PTAL.

@meteorcloudy
Copy link
Member

Wow, @fmeum you are awesome!

I tried to build the site internally, almost worked!
Some findings:

  • From build/style-guide, click 7.2 jumped to versions/7.2.0//build/style-guide which resulted a 404, should be versions/7.2.0/build/style-guide
  • From versions/7.2.0/build/style-guide, click 7.1 jumped to versions/7.1.0//versions/7.2.0/build/style-guide
  • No links found for 6.4 and 6.5, is that intended?

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 13, 2024

  • From build/style-guide, click 7.2 jumped to versions/7.2.0//build/style-guide which resulted a 404, should be versions/7.2.0/build/style-guide

This should be fixed, I had an extra slash.

  • From versions/7.2.0/build/style-guide, click 7.1 jumped to versions/7.1.0//versions/7.2.0/build/style-guide

Did you regenerate the 7.2.0 docs with this change? It's required to set the original_path var correctly. I could get rid of this requirement (and simplify the overall PR) if I had a way to strip the versions/X.Y.Z prefix. That would at least require checking for a fixed prefix and slicing a string (assuming all reasonable Bazel version numbers are 5 digits long).

  • No links found for 6.4 and 6.5, is that intended?

What happens when you click on the 6.4? Do you see it?

@meteorcloudy
Copy link
Member

This should be fixed, I had an extra slash.

I can confirm!

That would at least require checking for a fixed prefix and slicing a string (assuming all reasonable Bazel version numbers are 5 digits long).

Until we reach bazel 10? I have to check closer is there is any other way..

What happens when you click on the 6.4? Do you see it?

Oh, I meant there is no version list on versions/6.5.0/build/style-guide

image

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 13, 2024

Could you share the generated code for some page for 7.2.0? It looks like the output_path variable isn't set at all and falls back to the request path.

How did you build the 7.2.0 docs?

Based on the picture, the _buttons.html file isn't included in the Bazel 6 branches and so the part that adds the version list isn't loaded. Maybe the _buttons.html addition could be cherry-picked or added manually during docs regeneration?

@meteorcloudy
Copy link
Member

image

@meteorcloudy
Copy link
Member

How did you build the 7.2.0 docs?

This is an internal tool to build everything, I don't know the exact details.

@meteorcloudy
Copy link
Member

And your are right, _buttons.html doesn't exist for 6.x, we can check how to fix that later.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 13, 2024

Could you also share the raw .md file as produced by bazel build //scripts/docs:gen_release_docs --config=docs?

For example, when I create a new release-1.2.3 branch based on this PR and run that command there, then the file at versions/1.2.3/contribute/breaking-changes.md in the generated archive contains this:

{% dynamic setvar version "1.2.3" %}
{% dynamic setvar original_path "/contribute/breaking-changes" %}
{% include "_buttons.html" %}

For some reason, it looks like the docs you generate do not have original_path set. Could you share the raw .md generated by that pipeline?

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 13, 2024

Alternatively, something may be wrong with this snippet in the PR:

{% dynamic if not setvar.original_path %}
{% dynamic setvar original_path %}{% dynamic print request.path %}{% dynamic endsetvar %}
{% dynamic endif %}

The behavior you see would also be consistent with the if condition always being false even though original_path is set by all pages before including _buttons.html. But it does seem to work here:

{% dynamic if not setvar.source_file %}

@meteorcloudy
Copy link
Member

I see the problem now, each version has a forked version of _buttons.html which wasn't changed during I import this PR. I guess I can adjust that manually.

@meteorcloudy
Copy link
Member

OK, the 7.2.0 _buttons.html should still be generated from the root version. I'm not sure what's happening now. @fweikert

@fweikert
Copy link
Member

Hey Fabian,

Great work, thank you! I'm working on a change that sets original_path for all existing versioned docs, then it should work.

Copy link
Member

@fweikert fweikert left a comment

Choose a reason for hiding this comment

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

The scripts also need to update _buttons.html to add the version selector for the new release.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 14, 2024

The scripts also need to update _buttons.html to add the version selector for the new release.

Do you want me to do that? Not sure how as that change to the file would also need to be committed.

@fweikert
Copy link
Member

Technically it should be enough to include the modified _buttons.html file in the zip file generated by //scripts/docs:gen_release_docs at the root level, and then the release scripts take care of committing it. But that's something we can do in the future.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 17, 2024

Probably that should be done in the same place as where _toc.html is updated. I don't know where that is though.

@fweikert
Copy link
Member

There is a genrule that invokes a dedicated Java binary to update the TOC: https://github.com/bazelbuild/bazel/blob/master/scripts/docs/BUILD#L30

For _buttons.html we might get away with a simpler Python script.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 18, 2024

I updated the TOC updater to also update the version indicator.

@fmeum fmeum requested a review from fweikert June 18, 2024 08:52
@fweikert
Copy link
Member

I added version and original_path variables to the old versioned docs, so everything should work now.

@fmeum
Copy link
Collaborator Author

fmeum commented Jun 18, 2024

@fweikert Thanks! Is there anything else I should do or is this ready to be imported?

@meteorcloudy
Copy link
Member

We are importing this PR, nothing else needed, thanks!!

site/en/bazel.css Show resolved Hide resolved
@github-actions github-actions bot removed the awaiting-review PR is awaiting review from an assigned reviewer label Jun 18, 2024
@meteorcloudy
Copy link
Member

The changes are live: https://bazel.build/versions/7.2.0/rules/lib/globals/build

Thank you so much for fixing this! @fmeum 🙏🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Documentation Documentation improvements that cannot be directly linked to other team labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants