-
Notifications
You must be signed in to change notification settings - Fork 50
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
Fixed background option for window and elements #51
Conversation
- Now background can be set even if it isn't specified a theme
This is applied when theme is dark or it has a custom background
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.
Thank you for taking the time to make changes and submit a pull request. For now, I am going not going to merge it, for reasons I mention in the review and in #50.
This library is currently used by more people than I had ever expected, and I want it to be the best possible. Improvements are needed, but I think they need to be more substantial, to guarantee consistent behavior across different themes.
|
||
def set_background(self, background): | ||
self.config(background=background) | ||
ttk.Style(self).configure(".", background=background) |
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 is a theme option that I would prefer to set in the theme Tcl
files instead for themes for which it is appropriate and has not already been set. If the user wants to change the background color of all the widgets in the instance, they should use this method themselves.
|
||
def set_theme(self, theme_name, toplevel=False, background=False): | ||
"""Redirect the set_theme call to also set Tk background color""" | ||
ThemedWidget.set_theme(self, theme_name) | ||
color = ttk.Style(self).lookup("TFrame", "background", default="white") | ||
if background is True: | ||
if bool(background) is True and color == "#f5f6f7": |
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.
I am not quite comfortable with this implementation. I would really much prefer to keep a bool
type keyword argument, as I mention in #50 . Also, it is unconventional to hard-code colors or other values like this.
Co-Authored-By: maicol07 <[email protected]>
With PR #58 , I merged a set of changes that fixes this issue. |
Issue: #50