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

Add [note], [warning], [important] and [tip] blocks to the editor help #63079

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jul 16, 2022

Follow-up to #62710.

Benefits include:

  • Uses a different color for note and warning blocks to make them easier to distinguish from other text.
  • Ensures consistent formatting throughout the documentation.

Preview

Screenshot_20230713_175309

Screenshot_20230713_175512

Screenshot_20230713_175151

Screenshot_20230713_175141

@Calinou Calinou requested review from a team as code owners July 16, 2022 17:23
@Calinou Calinou added this to the 4.0 milestone Jul 16, 2022
@Calinou Calinou force-pushed the editor-help-add-note-warning-blocks branch 2 times, most recently from a3033b6 to f828b66 Compare July 16, 2022 17:30
@Calinou Calinou marked this pull request as draft July 16, 2022 17:30
@YuriSizov
Copy link
Contributor

Oh, nice idea. Though the yellow for the light theme can probably be more saturated (and darker?).

@Calinou
Copy link
Member Author

Calinou commented Jul 16, 2022

Oh, nice idea. Though the yellow for the light theme can probably be more saturated (and darker?).

It might be worth tweaking the general warning_color for this (also error_color). In general, it's hard to have yellow that is readable and stands out on a light theme though.

editor/editor_help.cpp Outdated Show resolved Hide resolved
@Calinou Calinou force-pushed the editor-help-add-note-warning-blocks branch from f828b66 to 91749b3 Compare July 17, 2022 22:22
@Calinou Calinou marked this pull request as ready for review July 17, 2022 22:22
@Calinou
Copy link
Member Author

Calinou commented Jul 17, 2022

I made the text localizable and added make_rst.py support:

2022-07-18_00 22 08

2022-07-18_00 22 33

Copy link
Contributor

@YuriSizov YuriSizov left a comment

Choose a reason for hiding this comment

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

Looks good!

Besides the notes that I've left, there is an issue with the fact that it's a block now. In blocks we allow new lines, and the built-in docs handle this fine
godot windows tools 64_2022-07-19_14-40-48

But the RST generation does not, as the new lines are not indented appropriately.
Code_2022-07-19_14-42-49

We should either strip the new lines, or support it correctly.

@@ -1016,6 +1016,7 @@ def rstize_text(text, state): # type: (str, State) -> str

# Handle [tags]
inside_code = False
inside_note_warning = False
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not used in any way. Should it? There is a bunch of logic attached to inside_code, but I guess there is no special parsing for these tags. So, no reason to add this?

tag_text = ""
tag_depth -= 1
inside_note_warning = False
# Strip newline if the tag was alone on one
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that it's copied from the above sample, but I don't get that comment. It feels like it's cut off, or it's a typo. Do you know what it actually supposed to mean and can you fix it (here and above too)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied that part without understanding what it was really useful for.


} else if (tag == "warning") {
// Warning block.
p_rt->push_color(warning_color);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why it works correctly, but we should probably add a special case for note/warning blocks, like we do for code blocks, and set a flag. So when we pop the tag from the stack (around line 1768 as of your PR), we properly pop the color.

I think it currently works without visible issues because a new color is pushed somewhere, but I don't think that we correctly handle the extra color here.

@akien-mga
Copy link
Member

akien-mga commented Jul 19, 2022

I'm all for better styling things which should be emphasized, but I wonder if the way we currently use [b]Note:[/b] in the XML isn't intended to provided less emphasis than what a full blown .. note:: admonition does in reST.

Having a level of emphasis similar to the reST one seems useful but then we might want to review all our existing [b]Note:[b]s and see if they really warrant such highlight, or if they should just be a plain paragraph.

@YuriSizov
Copy link
Contributor

I've also looked into other existing line prefixes that we have.

  • A couple of descriptions use [b]Node:[/b] instead of notes, which should be fixed in this PR 🙂
  • There are several [b]Performance:[/b], which may be changed for warnings? How about making the title of the note block customizable with a parameter that defaults to Note/Warning?
  • There are several [b]Example:[/b] which prepend some code blocks, but not all of them. Should we just remove them?
  • There are a couple of [b]FIXME:[/b] in there 😱
  • There is [b]Important:[/b] which should probably be a warning?
  • Then there are a bunch of one-ofs, which may or may not require the same treatment as I propose for "performance"

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 19, 2022

PS. I thought we fixed that indentation issue for subsequent paragraphs?

godot windows tools 64_2022-07-19_14-34-34

@akien-mga
Copy link
Member

There is [b]Important:[/b] which should probably be a warning?

I think reST has .. important::, could be worth supporting.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.x Feb 10, 2023
@akien-mga
Copy link
Member

Would be nice to revisit this.

I still have the concern from #63079 (comment), but I think we could start by adding support for admonitions like on the online docs (e.g. for top-level stuff like #79206), and evaluate in a follow-up how we want to make use of them for existing class ref descriptions.

In particular, we'd have to see how the online docs would look like if we were to convert all [b]Note:[/b] to .. note::.

@Calinou Calinou changed the title Add [note] and [warning] blocks to the editor help Add [note], [warning], [important] and [tip] blocks to the editor help Jul 13, 2023
@Calinou Calinou force-pushed the editor-help-add-note-warning-blocks branch from 91749b3 to ec8e78b Compare July 13, 2023 16:25
@Calinou
Copy link
Member Author

Calinou commented Jul 13, 2023

I've rewritten the implementation from scratch, with new [important] and [tip] admonitions and icons for each admonition. See OP for updated screenshots.

I've marked the PR as draft as there are 2 outstanding issues:

  • I need to replace all existing blocks in the XML again.
  • Multi-paragraph admonitions are not formatted correctly in the rST output (but they are displayed correctly in the editor). Subsequent paragraphs are missing the 4-space indentation required at the beginning of each line. I'm not sure how to implement this.

@Calinou Calinou marked this pull request as draft July 13, 2023 16:28
@Mickeon
Copy link
Contributor

Mickeon commented Jan 14, 2024

Is there still interest for this PR? I would absolutely love this, for one.

@Calinou
Copy link
Member Author

Calinou commented Jan 15, 2024

Is there still interest for this PR? I would absolutely love this, for one.

I don't know how to address the remaining issue, so feel free to salvage this if you'd like. It would be hugely appreciated 🙂

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.

4 participants