-
-
Notifications
You must be signed in to change notification settings - Fork 674
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 support for icons on buttons #2310
Conversation
58c0dc8
to
826203d
Compare
826203d
to
6463fd8
Compare
core/src/toga/widgets/button.py
Outdated
@icon.setter | ||
def icon(self, value: IconContent) -> None: | ||
if isinstance(value, toga.Icon): | ||
icon = value | ||
else: | ||
icon = toga.Icon(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows us to not include
None
as a valid value when setting an icon on Button (as you can't set no button and no text, and setting the icon to None on an existing button would leave the button in an unknown state).
The setter type hint isn't visible in the documentation, so that makes it look like the setter does accept None. And I think there's a reasonable interpretation of that: if the button is already displaying text, then keep it unchanged, or if it's displaying an icon, remove it and switch to the empty string. That way, reading the property and then writing it back would always be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should also explicitly say that setting the icon will remove the text, and vice versa.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the button is already displaying text, then keep it unchanged
It still doesn't look like this is implemented or tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 Fixed now.
is there a way to set a size for the icon on the button? or have it scale with the size of the button? |
@l-o-u-i-s-g Not as currently implemented. Icon buttons have a fixed size - 32x32 on desktop; 48x48 on mobile. The biggest complication around implementing resizable icon buttons is maintaining the relationship between the image providing the icon and the requested size - does the size of the image inform the size of the button, or vice versa? This might be a complication that could be resolved, but it's not high on my personal list to do so. |
how do you handle the size for the font? why should it be different for the icon? |
actually just thought of an easy way to have icons that can be sized and colored. pip install fontawesome example:
|
This is one of the most common feature requests/support queries for Toga. Since nobody has taken the bait of trying to implement it, and it's going to be useful for the camera shutter button on the macOS Camera implementation, I figured I might as well bite the bullet.
Implemented on all platforms, although the web and textual implementations are stubs because icons don't work on web, and probably can't work on textual.
Makes a small change to the IconContent type declaration, separating None from the definition. This allows us to not include
None
as a valid value when setting an icon on Button (as you can't set no button and no text, and setting the icon to None on an existing button would leave the button in an unknown state).Fixes #774.
PR Checklist: