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 color in built-in documentation for overridden properties #87255

Merged

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Jan 16, 2024

Should mitigate #84796 to an extent, and perhaps supersedes #84939?

The name is pretty self-explanatory.
It uses the warning color. This is used pretty consistently for other "inheritance"-related concepts in the editor, so I think it makes sense.

. .
image image

I was also asked for a bit of padding between overridden and normal properties, although... I still don't like the way it looks, but hey, it works.

@Mickeon Mickeon requested a review from a team as a code owner January 16, 2024 15:57
@Mickeon Mickeon force-pushed the documentation-overridden-property-colour branch 3 times, most recently from eaf83c4 to 5444c37 Compare January 16, 2024 16:03
@Calinou Calinou added this to the 4.x milestone Jan 16, 2024
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 17, 2024

The test failed because the generated class reference would need to be updated to move the overridden properties above all else. In my opinion, the sorting of this PR does makes sense, but that's a lot of multiple, small changes that would need to be done, and I'm not sure if this PR is desired just yet as is.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 17, 2024

I think this should only be on the display side, unless they're put in a separate tag I don't think we should do complicated sorting like this, you should be able to iterate over them twice when displaying, slightly unnecessary processing but the most trivial method IMO

@YuriSizov
Copy link
Contributor

I think this should only be on the display side, unless they're put in a separate tag I don't think we should do complicated sorting like this, you should be able to iterate over them twice when displaying, slightly unnecessary processing but the most trivial method IMO

Yeah, I agree with this. This change shouldn't require precise ordering in the XML files. It should just be a visual enhancement.

@Mickeon Mickeon force-pushed the documentation-overridden-property-colour branch 2 times, most recently from 37a8b00 to 99a1fbb Compare January 17, 2024 12:52
@Mickeon
Copy link
Contributor Author

Mickeon commented Jan 17, 2024

Yyeah, the thing that peeved me most was the "unnecessary" second sorting pass, but if that is tolerable, so be it. Done.

@Mickeon Mickeon force-pushed the documentation-overridden-property-colour branch from 99a1fbb to b93a101 Compare February 11, 2024 15:46
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 11, 2024

Rebased after minor conflict. Would be nice to have sooner than later.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, with some suggestions

editor/editor_help.cpp Outdated Show resolved Hide resolved
editor/editor_help.h Outdated Show resolved Hide resolved
@Mickeon Mickeon force-pushed the documentation-overridden-property-colour branch from b93a101 to fbc584d Compare February 11, 2024 16:30
@Mickeon Mickeon requested review from a team as code owners February 11, 2024 16:30
Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Implementation looks ok.

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Feb 12, 2024
@akien-mga akien-mga merged commit f879160 into godotengine:master Feb 12, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Mickeon Mickeon deleted the documentation-overridden-property-colour branch February 13, 2024 15:55
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.

6 participants