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

Update language on gui_theme_type_variations.rst #10505

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lackhand
Copy link

I was very confused about how to use themes, and felt the documentation didn't resolve my confusion.

  1. Tighten up the language 28-32 referring to extending types (if you're not familiar, its use of the specialized word "type" ("theme type") could refer to a "type system", ie, class extension for a new kind of Node rather than type variation). By ensuring "type" is not used raw but "type variation" or "theme type", we discourage that use.
  2. Add an explanatory paragraph 42-43 to creating a type variation that explains every step is necessary and summarizes what they are. Without this it is unclear how critical the para 54+ is -- count the number of "cans" and "shoulds", which makes it sound very optional!

Thank you!

I was very confused about how to use themes, and felt the documentation didn't resolve my confusion.

1. Tighten up the language 28-32 referring to extending types (if you're not familiar, its use of the specialized word "type" ("theme type") could refer to a "type system", ie, class extension for a new kind of Node rather than type variation). By ensuring "type" is not used raw but "type variation" or "theme type", we discourage that use.
2. Add an explanatory paragraph 42-43 to creating a type variation that explains every step is necessary and summarizes what they are. Without this it is unclear how critical the para 54+ is  -- count the number of "cans" and "shoulds", which makes it sound very optional!

Thank you!
Comment on lines +42 to +43
Type variations are created from the theme editor and require both a name and
a base type from which to extend.
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding this seems redundant to me since the rest of the section walks you through using the theme editor to do this, and choosing a base type.

Copy link
Author

@lackhand lackhand Jan 22, 2025

Choose a reason for hiding this comment

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

I'm very open to a different fix, but I believe some kind of fix is warranted.
My PR description explains my objection to the rest of the section: it presents itself as optional and skippable if you are not already aware of exactly how the feature works.

Much much much more description below.

The rest of the paragraph includes the following snippets, emphasis mine:

  1. "You can select the base type there"
  2. "typically the name of a control node class"
  3. "you should now be able to see its properties ..."
  4. "you can edit them as normal"

Note that one might come away from this thinking that they don't need to specify a base type (the use of "can" in points #1, where it means required, and #4, where it means optional).

I suppose another fix would be to tighten up the language:

  1. "You must select a base type there"
  2. "typically the name of a built-in control node class" (edit: Is this accurate language? I don't know the right terms. Merely extending a node in gdscript isn't enough afaict to work with this feature.)
  3. I'm not sure how to fix this one! I notice that if you chain custom types -- Foo extends Label and Bar extends Foo -- Bar doesn't show any properties. It might be cosmetic, I'm not sure. Together with point #2, how the user should proceed is pretty unclear IMO. So: "You should now be able to see properties if you inherited from a control node, which is recommended for beginners"? Still messy, maybe both sentences get a rewrite.
  4. "you can edit them as normal" (no notes, this is fine 😁!)

Given all of that, I didn't try to fix the language, and instead just added a guiding "goal" statement; in jokier language "you're here to use the UI to add a theme and set its base type, without which it might not work. There are other reasons it might not work too, which are buried in the details. Good luck."

If this is unpersuasive, perhaps there's a better fix here? But the feature is complex enough that being clear about the absolute requirements seems good to me.

tutorials/ui/gui_theme_type_variations.rst Outdated Show resolved Hide resolved
@lackhand
Copy link
Author

lackhand commented Jan 22, 2025

Thank you very much for your time and attention!
FWIW I respond very well to explicit pushback along the lines of "skyace has bigger fish to fry than this so lackhand should wait until it's worth fixing this" -- I'm fine leaving this branch open for decades until the glorious future where this is our biggest problem (if you also smell something wrong here but don't like my proposed fixes) 😁
And maybe on this project that's done by closing the PR? Ok, that's fine too, lmk.

@lackhand lackhand requested a review from skyace65 January 22, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants