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

Remove font height restriction from Button #60867

Merged
merged 1 commit into from
May 17, 2022
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented May 7, 2022

Possible fix #35005
The button height is determined by the text buffer, so additinally using font height is not necessary.
godot windows tools 64_PERzIDkiyp

@KoBeWi KoBeWi added this to the 4.0 milestone May 7, 2022
@KoBeWi KoBeWi requested a review from a team as a code owner May 7, 2022 22:28
@Calinou
Copy link
Member

Calinou commented May 7, 2022

How does this behave when the button is within a Container with no custom minimum size set?

@KoBeWi
Copy link
Member Author

KoBeWi commented May 7, 2022

It shrinks to the stylebox size (the same as show in the GIF). This change only affects buttons without text, so it doesn't break anything in majority of the cases.

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. However, we have to keep in mind that this has an effect on the editor UI (vertical shrinking), which might require some adjustments. For example, the OptionButton arrow isn't centered anymore in some cases and a few controls which had the same height before now have different heights. Both observable here:
Before:
grafik
After:
grafik

GIF before/after of whole editor:
2

Copy link
Contributor

@fire-forge fire-forge left a comment

Choose a reason for hiding this comment

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

Not a bad idea. Could be useful for icon-only buttons where the font height is taller than the icon.

@KoBeWi
Copy link
Member Author

KoBeWi commented May 8, 2022

@Geometror OptionButton will be handled by #59303, other buttons don't really have this problem.

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

I only checked the most common areas, but altough some elements shrink in height (as seen in the GIF) they still look good. There is only one nitpick, the plus icon of the "Add new scene" button in the central tab bar moves up 1 px, making it look a bit off-centered :)
grafik

@Geometror
Copy link
Member

Just noticed that some Color inspector properties are squished heavily:
Before->after:
grafik

@KoBeWi
Copy link
Member Author

KoBeWi commented May 8, 2022

Maybe bruvzg's suggestion makes sense actually...

EDIT:
Ok changed it so that the height is ignored only when the text is empty. This should fix all shrinking issues.

@YuriSizov
Copy link
Contributor

Just noticed that some Color inspector properties are squished heavily: Before->after: grafik

This is actually a good thing and the fix working as intended, IMO. ColorPickerButton shouldn't depend on the height of the font, and we should just adjust its theme properties to give it more initial height to match the UI.

@Geometror
Copy link
Member

The shrinking is still present (at least in color theme overrides section) but I agree with what @YuriSizov said, this could be fixed by adjusting the ColorPickerButton style/minsize (in this or a subsequent PR).

@akien-mga
Copy link
Member

So from the above discussion it sounds like there are still pending issues to solve before merging this?

@KoBeWi
Copy link
Member Author

KoBeWi commented May 17, 2022

There are some tweaks that need to be done to buttons that don't have any text, but it could be left for a subsequent PR. Not sure what's preferred; it's not directly related to the changes here.

@Geometror
Copy link
Member

I think this can be merged. The tweaks needed for the editor UI can be done separately.

@akien-mga akien-mga merged commit 3ad751f into godotengine:master May 17, 2022
@akien-mga
Copy link
Member

Thanks!

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.

godot buttons can't be resized to 8×8
7 participants