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

Further separate icon from text of buttons in both editor and default themes #80285

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

YeldhamDev
Copy link
Member

I've noticed that the the icon and text separation in buttons seems a little too modest, making it look like there's almost no separation at all. This PR makes so that the default separation is 4 instead of 2, and it also removes the BottomPanelButton theme variation, as I don't feel like it's needed after this change.

Before: After:
image image
image image
image image

@Maran23
Copy link
Contributor

Maran23 commented Aug 5, 2023

A separation of 4 is also the default on other UI libraries like JavaFX. So it seems like a good choice.

@Calinou
Copy link
Member

Calinou commented Aug 6, 2023

This will change existing projects' theme appearances if they don't override the constants in question. I think it's a good change, but it's unfortunately technically a breaking change 🙁

@YeldhamDev
Copy link
Member Author

@Calinou This has already been talked about in the dev chat, and acceptable for a minor release: https://docs.godotengine.org/en/stable/about/release_policy.html#what-are-the-criteria-for-compatibility-across-engine-versions

@Calinou
Copy link
Member

Calinou commented Aug 7, 2023

@Calinou This has already been talked about in the dev chat, and acceptable for a minor release: docs.godotengine.org/en/stable/about/release_policy.html#what-are-the-criteria-for-compatibility-across-engine-versions

That particular note is for the default project theme, not user-made themes that happen not to override the h_separation constant.

Either way, the visual difference with this PR is small, except in games with a pixel art appearance that use the viewport stretch mode and a low base resolution. For instance, increasing icon spacing from 2 to 4 pixels on a 340×240 canvas is pretty significant in comparison to performing the same at a 1152×648 base resolution.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Seems like a good change to me.

As long as we document it clearly in the "Changes" section of the changelog, should be fine for 4.2.

@YeldhamDev YeldhamDev changed the title Further separate icon from text of buttons in both editor and default themes Further separate icon from text of buttons in the editor theme Aug 7, 2023
@YeldhamDev
Copy link
Member Author

@akien-mga Just as I removed the default theme changes... 🥲

@YeldhamDev YeldhamDev changed the title Further separate icon from text of buttons in the editor theme Further separate icon from text of buttons in both editor and default themes Aug 7, 2023
@akien-mga
Copy link
Member

@akien-mga Just as I removed the default theme changes...

Well I didn't consider it as an option but yes, it could make sense to preserve compat for the default theme and only change the editor theme.

@YeldhamDev
Copy link
Member Author

YeldhamDev commented Aug 7, 2023

Well, now I just reverted them back. So I hope we can just go with it...

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.

I think it's fine to change this for a minor release. Better defaults are always good, and the ability to override them is not going anywhere.

Both default and editor themes look fine IMO, nothing stands out:
image

@akien-mga akien-mga merged commit f6b9d44 into godotengine:master Aug 9, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YeldhamDev YeldhamDev deleted the just_a_little_bit branch August 9, 2023 13:51
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.

5 participants