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

Merge docs-section/docs-page templates into docs #1149

Merged
merged 6 commits into from May 14, 2024

Conversation

doup
Copy link
Contributor

@doup doup commented May 3, 2024

Merge docs-section/docs-page templates into docs leveraging default filter (allows removal of repetitive if section … elseif page … code).

Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

LGTM.

I wonder if we could use this same feature to merge docs-page and docs-section into docs-base and just rename it to docs.html. I want to say the primary reason we even have a separate section and page templates for the docs / book portion of the site is because it was easier and more readable than writing even more if else conditions for page and section variables, but this solves that issue.

@doup
Copy link
Contributor Author

doup commented May 4, 2024

Hehe, that's actually something I wanted to ask in Discord as it seems that those are just repeated. I was going to open a PR removing those templates and just leaving a docs.html (former docs-base).

@doup doup changed the title Simplify docs-base.html by using default filter Merge docs-section/docs-page templates into docs by using default filter May 4, 2024
@doup doup requested a review from TrialDragon May 4, 2024 18:37
@doup
Copy link
Contributor Author

doup commented May 4, 2024

I wanted to try an extra simplification by moving the section_or_page (previously current) variable declaration to the root layout (base.html). And after seeing it worked fine I took the extra step of also removing the docs-section/docs-page templates.

This increases the size of the PR, but it's quite easy to follow as it's mostly repetitive changes. Let me know if you prefer this in two separate PRs, I can revert the changes.

@TrialDragon
Copy link
Member

The scope is fine imo.

Copy link
Member

@TrialDragon TrialDragon left a comment

Choose a reason for hiding this comment

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

LGTM. I almost thought you also had to change a template variable in generate-release, but I guess we currently don't include that in it's front matter and add it when we move it to be an actual page.

@doup
Copy link
Contributor Author

doup commented May 4, 2024

I'm tempted to slip these changes in layout/base.html, opinion? 😇
https://github.com/doup/bevy-website/pull/3/files

@TrialDragon
Copy link
Member

Mm, tentatively I'll say that perhaps the layout/base.html changes can be it's own PR for sake of keeping the PR from slowly growing too big, but I approve of them either way.

@doup
Copy link
Contributor Author

doup commented May 5, 2024

I'll leave this PR as it is and I'll open the base.html afterwards.
Thanks for the feedback!

@doup doup changed the title Merge docs-section/docs-page templates into docs by using default filter Merge docs-section/docs-page templates into docs May 6, 2024
@TrialDragon TrialDragon added the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label May 8, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 13, 2024
@TrialDragon
Copy link
Member

This PR needs to be updated to account for the contributing section added in 11f5816 since it is still using docs-section.html.

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label May 13, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 14, 2024
Merged via the queue into bevyengine:main with commit 50b1760 May 14, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants