-
-
Notifications
You must be signed in to change notification settings - Fork 605
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
Defaulting Element Styling #1689
Conversation
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.
Thanks for this PR, @natankeddem!
The approach looks very promising. I just made a few changes:
- We can define the
_default_props
as a static dictionary (as you did in an earlier commit). This avoids the need forhasattr
etc. The crux might be to copy the dictionary when subclassing. Otherwise the default props of, e.g., a button would modify the default props of all other elements, because they share the same dictionary instance. (The advantage with copying the dictionary when subclassing is that elements inherit default props from a parent class, which is rather intuitive in my point of view.) - I added a pytest to check various requirements, like what happens when subclassing buttons etc.
- I moved the demo into element_documentation. Even though it only contains buttons, it demonstrates a common feature for all UI elements.
- I simplified the demo to focus on the essential use case, and added additional information to the description.
Now let's add default classes and style as well. 😎
Changes look good, thank you. The copy in |
@natankeddem Oh we can do it on this PR. Props, classes and style are so closely related (but don't overlap), they don't need to be split into separate PRs. And maybe we spot something that applies to props as well, e.g. a hint in the documentation about styling. So let's continue on this PR. |
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.
Looks like we're ready to merge. 🙂
I only simplified the new methods a bit (we don't need the change detection anymore since we don't call update()
), removed the ";" from the test classes and tweaked the demos a tiny bit.
Thanks again, @natankeddem, for you ongoing support!
Looks good, thank you for helping me through this! |
Per discussion #1683 here is a POC for defaulting props. This will let clusters of elements or full pages to be set to the same props. The
default_props
method retains all the same arguments and functionality of theprops
method. Note thatdefault_props
modifies elements during__init__
therefore after instantiation elements will no longer be modified by changes initiated bydefault_props
to the class attribute.If this looks promising I can extend this to classes and/or style as well per the original discussion.