-
-
Notifications
You must be signed in to change notification settings - Fork 691
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 "horizontal" and "vertical" aliases for align_items and justify_content #3113
Conversation
I'll look at the MicroPython failure tomorrow, but any other review comments would be useful. |
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 implementation of the aliasing behavior makes sense; a couple of minor notes inline.
However, reading how this is documented (in the changenote and the docs for Pack), I've got some pedagogical concerns about this change. See #3113 for further discussion.
try: | ||
_all_properties = style_cls._BASE_ALL_PROPERTIES | ||
except AttributeError: | ||
# Travertino 0.3 compatibility | ||
_all_properties = style_cls._ALL_PROPERTIES |
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.
It looks like MicroPython doesn't support the __init_subclass__
method used by BaseStyle, so this will have to be revisited once we start actually running Travertino on MicroPython rather than simply importing it.
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.
Ugh -_-
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.
Actually turns out it's super simple, I can just replace the subclass_init with a pair of properties.
Co-authored-by: Russell Keith-Magee <[email protected]>
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.
After the clarifications in the discussion on #3111, I think this is now good to go.
PR Checklist: