-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Overhaul the top sections of the class reference (GUI classes) #76702
Overhaul the top sections of the class reference (GUI classes) #76702
Conversation
446590e
to
7244611
Compare
7244611
to
dc31f19
Compare
e6fabeb
to
0d68bd3
Compare
4cc4b8f
to
2547095
Compare
doc/classes/ColorRect.xml
Outdated
</brief_description> | ||
<description> | ||
Displays a rectangle filled with a solid [member color]. If you need to display the border alone, consider using [ReferenceRect] instead. | ||
Displays a rectangle filled with a solid [member color]. If you need to display the border alone, consider using a [PanelContainer] instead. |
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.
I think suggesting ReferenceRect was correct. It can be displayed at runtime too.
Also PanelContainer is a container. It's a completely different thing. If anything, it should be Panel (can be in addition to ReferenceRect).
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.
Idk, I don't feel good telling people to use something that's intended as an editor hint (even the class name says it's just "a rect for reference"). The ability to show up in-game wasn't even present in older versions, and editor-only is by default. Even the "in-game display" I would say is also meant for debugging purposes, not to have borders in the final product.
I'll leave this up as unresolved for now but I'll fix the PanelContainer mistake.
Ok finished reviewing. GitHub started to lag near the end 😬 I really think that you should remove all description sentences that repeat brief description. idk if we have any official policy against that tho. |
It's a common pattern that was decently present already. In general, I feel like I reduced the number of times it was done, rather than increasing it, maybe my PR for GUI classes is the exception... @KoBeWi since I was behind your tail fixing up and commenting on your review this whole time, could you go back and address some of my comments? |
p.s. I educated myself on the difference between input fields and editors. I think TextEdit, CodeEdit, and GraphEdit easily pass the bar needed to count something as an editor. |
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.
Github why did you start a review, I was just responding to KoBeWi
Will do. Not right now, but surely tomorrow. |
2547095
to
45a6b6d
Compare
Timezone buddies! :3 |
b5f7c00
to
2215ee5
Compare
Thanks for the review! I think I'm done addressing its different aspects and I feel good about these changes once more. |
57d994e
to
8f15542
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.
Descriptions look good to me. See #76526 (review) about the "A"/"An" discussion, which applies to this PR as well.
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.
See discussion about A/An, otherwise looks good to me. :)
8f15542
to
151a4ba
Compare
Thanks! |
Cherry-picked for 4.0.4. ViewportTexture changes to the note at the end depend on another PR, so I skipped those. |
The top sections of classes are often one of the first things users read about them, so they are important to get right.
Part of a series; this is the one dedicated to the GUI & Text team.
I mostly touched up the short descriptions and occasionally the full descriptions and the tutorials sections. I also tweaked a little bit of documentation to the side and added a few missing descriptions.
I also added TextServerDummy documentation, but split that off to a different PR: #76699
Rules for brief descriptions
I'm abiding by a set of rules for consistency. I had them in mind when writing all descriptions, but violated them intentionally sometimes.
Use the articles "a" and "an". This is helpful for words that can be multiple things, i.e. "light" which can be a noun, an adjective, and a verb. This, and the fact it's grammatically correct, helps with reading comprehension in many cases.
Use a single sentence with punctuation. If two sentences are needed, don't use newline.
Never use text formatting (i.e. italics) or external links.
Don't use capitalization for normal words. If referring to a class, use a reference.
For nodes, always answer "What is this?". For non-node objects, it's also OK to answer "What does this do?"
Avoid self-referencing, such as "Popup: Popup is a [...]"
Avoid circular reasoning such as "CanvasLayer: Canvas drawing layer."
When a class is abstract, start with "Abstract base class for [...]".
I often used "Holds X" and "Stores X" to replace "Contains X" - I think it improves clarity that X is the main thing, not just one of the things the resource holds.
Try to stay under 120 characters without compromising the quality of writing. (Sometimes it's impossible, in that case, go on for a bit longer)
Thanks a ton to everyone who participated in a 3+ hour long discussion about how to describe Viewport!