Fix font_hover_pressed_color and icon_hover_pressed_color not working for no stylebox use on Button
#97335
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…king for no stylebox use on `Button` You now don't need to have a `hover_pressed` stylebox to make use of `font_hover_pressed_color` and `icon_hover_pressed_color`.
KoBeWi
approved these changes
Oct 23, 2024
There was a problem hiding this comment.
I think the code was intentional. Button has toggle mode disabled by default, unlike CheckButton and CheckBox, for which the code was meant. The logic just existed in the parent class.
However I see no reason why it has to be like that. The colors are defined for Button, so the buttons still look correctly after this change. These colors were basically unused, which is weird.
|
Thank you! And congratulations on your first merged contribution! 🎉 |
|
Thank you and I really appreciete having it reviewed and merged. Have a good one! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #83393
I encountered this as I was working on my own project and I highly doubt this is a design choice. The issue is basically that the Button node cannot make use of
font_hover_pressed_colorandicon_hover_pressed_colorunless ahover_pressedstylebox is assigned to it. This is especially noticeable whenever you are working with toggleable buttons. I now changed it so that you don't need to have ahover_pressedstylebox to make use offont_hover_pressed_colorandicon_hover_pressed_color.Code
I think it is evident this was an oversight in the previous code judging by the commented line which suggests that the if statement was put in place as an edge case for CheckButton and CheckBox. This if statement and the comment itself were most likely copied from the function
Button::_get_current_stylebox()of the same file (old line 145) which does need to make sure the proper stylebox is assigned. In our case however, we do not need the stylebox in order to make use of this feature so I simply removed the if statement. As you can see, CheckButton and CheckBox are unaffected by this change so the if statement didn't have a reason to be there in the first place.