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: support overview page #365

Merged
merged 9 commits into from
Apr 1, 2024
Merged

feat: support overview page #365

merged 9 commits into from
Apr 1, 2024

Conversation

dandhlee
Copy link
Collaborator

@dandhlee dandhlee commented Apr 1, 2024

Adds support for summary_overview.md page. Gives users an option to also have overview_content.md file in their libraries, and if found, that content will be prepended to the existing Overview page content.

summary_overview.md page will be generated separately from the tool, through Synthtool. Same for overview_content.md file which will be supplied by library maintainers.

Adjusted a bit of the move_markdown_pages spacing.

Towards b/331981881.

  • Tests pass

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Apr 1, 2024
@dandhlee dandhlee marked this pull request as ready for review April 1, 2024 04:11
@dandhlee dandhlee requested review from a team as code owners April 1, 2024 04:11
@dandhlee dandhlee marked this pull request as draft April 1, 2024 04:11
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Apr 1, 2024
@dandhlee dandhlee requested a review from dansaadati April 1, 2024 04:12
@dandhlee dandhlee marked this pull request as ready for review April 1, 2024 04:14
@@ -280,7 +315,13 @@ def move_markdown_pages(

"reference.md", # Reference docs overlap with Overview. Will try and incorporate this in later.
# See https://github.com/googleapis/sphinx-docfx-yaml/issues/106.
"summary_overview.md", # Already included in the TOC.
"overview_content.md", # Will be included in the summary_overview page.

Choose a reason for hiding this comment

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

Nit: included in -> content will be merged into summary_overview page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

"overview_content.md", # Will be included in the summary_overview page.
]

# Use this to move the page but do not add them in the TOC.

Choose a reason for hiding this comment

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

Should this still be moved to be included in the reference docs after it is merged? If it's only intended to be dropped from the TOC can you just rm the file after it's been merged?

If so, I think you can remove the ignore_in_toc logic altogether.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, this is to (1) move the file from docs/summary_overview.md to output_location/summary_overview.md.
In the process we also (2) include it in app.env.moved_markdown_pages and (3) include it in the TOC as well.

We do need the summary_overview page in the output location for it to be used. However we don't need it in (2) or (3) because triggering either conditions will either (a) remove the page, or (b) include it again in the TOC.

# The content should be the same as base, and not throw any errors.
"tests/markdown_example_base.md",
"tests/markdown_example_does_not_exist.md",
"tests/markdown_example_dne_expected.md",

Choose a reason for hiding this comment

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

Nit: I think it's a bit clearer if you leave the expected result as tests/markdown_example_base.md because it makes it's clear it's the same file (so it has the same contents).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

Copy link
Collaborator Author

@dandhlee dandhlee left a comment

Choose a reason for hiding this comment

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

Thanks! No logic changes.

@@ -280,7 +315,13 @@ def move_markdown_pages(

"reference.md", # Reference docs overlap with Overview. Will try and incorporate this in later.
# See https://github.com/googleapis/sphinx-docfx-yaml/issues/106.
"summary_overview.md", # Already included in the TOC.
"overview_content.md", # Will be included in the summary_overview page.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

"overview_content.md", # Will be included in the summary_overview page.
]

# Use this to move the page but do not add them in the TOC.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, this is to (1) move the file from docs/summary_overview.md to output_location/summary_overview.md.
In the process we also (2) include it in app.env.moved_markdown_pages and (3) include it in the TOC as well.

We do need the summary_overview page in the output location for it to be used. However we don't need it in (2) or (3) because triggering either conditions will either (a) remove the page, or (b) include it again in the TOC.

# The content should be the same as base, and not throw any errors.
"tests/markdown_example_base.md",
"tests/markdown_example_does_not_exist.md",
"tests/markdown_example_dne_expected.md",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

@dandhlee dandhlee merged commit 9985d13 into main Apr 1, 2024
7 checks passed
@dandhlee dandhlee deleted the support_overview_page branch April 1, 2024 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants