-
Notifications
You must be signed in to change notification settings - Fork 42
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
Faq code subsections #50
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I think I understand what you mean when you were mentioning this idea in #48
I think that there is merit to your proposal, but don't think we should split all of the code questions into separate pages. The goal is to keep the related questions grouped together as we add more and more. At some point it might make sense to break the Code Questions FAQ into separate files like "Errors", "Conversions", "GDScript Equivalents", etc. But I don't think we have quite enough to warrant this.
One feature that you might not know is that currently each of the questions can be referenced by using a #{question-id}
which can be used to link them.
For example:
- https://godot-rust.github.io/book/faq/code.html#why-is-there-so-much-unsafe-in-godot-rust - https://godot-rust.github.io/book/faq/code.html#what-is-the-borrowfailed-error-and-why-do-i-keep-getting-it
I think that having a table of contents would definitely be useful in each FAQ page, but putting it all in the sidebar is a little messy. I checked out the PR and it clutters up the side bar and takes away from the sections.
Instead of putting it all in the sidebar, how about adding a list of all the questions as links at the top of each page. You can link to questions in code.md similar to the following:
- [What is the `BorrowFailed` error and why do I keep getting it?](#what-is-the-borrowfailed-error-and-why-do-i-keep-getting-it)
- [Why are methods on all Godot API classes using `&self` even when they should cause mutations?](#why-are-methods-on-all-godot-api-classes-using-self-even-when-they-should-cause-mutations)
- [Why is there so much `unsafe` in godot-rust?](#why-is-there-so-much-unsafe-in-godot-rust)
I think this would give us the best of both worlds as questions would be easily navigable in the Code Section while not cluttering up the sidebar.
@astrale-sharp While I appreciate the enthusiasm, this is now your 7th issue/PR within a couple of days. While this pull request is on the right track, it also created an new PR instead of using godot-rust/book#49 as requested. This increases a fair bit of management on our side, as well as confusion for other contributors, and makes progress generally harder to follow. For the time being, please don't create any new issues or PRs until the existing ones are merged. If you have ideas, please use one of your existing issues or Discord to discuss them. |
Okay! Sorry again 😖 |
Indeed I didn't know about this features and I think it makes a lot of sense having a table of content instead! |
b52bcde
to
c9c44db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor change and then we could be good to go. Optionally, if you have time, would it also be possible to do the same for the other parts of the FAQ? I believe that it would be quite useful for all the sections in addition toi the code subsection.
c9c44db
to
21c0314
Compare
Sure thing! |
I'm not super github-experienced, I just have to "request for review" right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the additions.
@Bromeon I'm satisfied with the changes. If you have any concerns, please let me know otherwise I'll go ahead and merge the PR.
Wonderful! Happy to help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the additions! 🙂
There's one broken link. Apart from that, this PR seems good for now.
It also shows that doing this manually is extremely brittle, even changing minor wording in a subtitle will break the links. The process also takes a lot of effort; one has to:
- click the section's title
- copy the URL part past the
#
and paste it to the ToC - copy the title's source code (not rendered) and paste it to the ToC
and think of all that whenever something changes, is reordered, added or removed.
In the future, we should look into automating this. mdbook-toc is the most promising solution I found in a quick search. This topic has been brought up countless times in mdbook
itself and ignored for over half a decade, so I think it's fair to say it would need a third-party solution or something we build ourselves.
21c0314
to
6809c5c
Compare
Okay, the link is now fixed but I would be interested to look into mdbook-toc so the code is more easily maintained. Should we wait and should i force push here when i get something working? |
Okay, this was extremly easy to set up, as you can see in my local branch here Please note that the depth level is defined in book.toml, in section [preprocessor.toc] by the field max-level and is currently set to 2 so it behaves the same as now. |
That looks like an excellent change, apologies for making you undo all the manual work. In that case, we should use the mdbook-toc so that the links are automatically preprocessed. That will save a lot of work in the future, if you want to add that to this and modify the table of contents, I'd be happy to test locally and then merge the changes. |
No worries! |
Sorry, didn't mean to create extra work -- I thought we could leave the manual ToC until we get to an automated solution. But if you already got something working, even better 👍🏼 Regarding PR, this is an annoying GitHub feature that it auto-closes PRs when you remove commits. I can't reopen it either, unfortunately. Maybe you can revert the deletion with this button? If that doesn't work, I found a manual how to restore the branch: |
I cant restore the branch since it was deleted and recreated again (so it already exist) but thanks to your link i can reopen my PR! |
6809c5c
to
f7c7fde
Compare
I have vanquished git! This should be good to go |
Awesome, just tried it out locally, seems to work great! 😀 One last request: since users now need to install $ cargo install mdbook
$ mdbook build to
? If possible, in the same commit ( |
f7c7fde
to
c1bdcc4
Compare
Great Idea! It's done! |
Looks good to me! |
Thanks much ❤️ |
Looks good to me! @astrale-sharp thank you for the help! I'll merge it now. |
In my opinion, code questions are extremely important and with that work, they are all visible in the index while reading the book. What do you think?