-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
UI creation API #1960
UI creation API #1960
Conversation
…ng to specific JS attributes
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
… a real server, that runs in local tests
…rent behaviour between local tests and CI (due to fake_server)
for more information, see https://pre-commit.ci
self.name = name | ||
self.allow_nones = allow_nones | ||
|
||
def __get__(self, obj, objtype=None): |
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.
out of curiosity, what's objtype
about?
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.
We can actually remove it. Here's an answer (even though it's actually not in the official docs even)
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.
In the docs it is called owner
and it is always the type of the instance. It is useful if the descriptor is accessed through the class itself (e.g. Foo.x) when instance will be None.
@@ -37,7 +37,7 @@ const python = [ | |||
"_path = None", | |||
]; | |||
|
|||
const ignore = new Ignore(python, "./pyweb"); | |||
const ignore = new Ignore(python, "-"); |
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 would be happy to remove that ignore
code / logic if we don't need to ignore anything anymore ... I mean, we can keep it around or commented but if we don't need it and we want to be sure everything we land in stdlib works in both Pyodide and MicroPython, maybe we can remove it? It doesn't have to happen in here, just wondering what you think about it, thanks.
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's an useful tool to manage incompatibilities. Imho, it'd be good to leave it documented and "in standby" in case we need in the future.
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.
Bravo @fpliger - this is awesome. 🎉
Take my comments with a pinch of salt... I'm just including them as an aide-mémoire for myself for potential future changes, should they be required. If I don't write them down, they're only in my head, and I'll likely forget them. 😉
|
||
def __set__(self, obj, value): | ||
if not self.allow_nones and value is None: | ||
return |
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.
Suggestion (feel free to ignore): raise ValueError
because None
isn't a valid value if self.allow_nones
is False
..? Something like this..?
if self.allow_nones and value is None:
raise ValueError(f"Cannot set a None value for {self.name}.")
The current code appears to not have any side-effects, so the programmer will just assume their None
value has been set, which is probably a dangerous assumption to make.
Errors should never pass silently.
(Zen of Python)
super().__init__(document.createElement(self.tag)) | ||
|
||
# set all the style properties provided in input | ||
if isinstance(style, dict): |
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.
How about..?
if style and isinstance(style, dict):
...
else:
raise ValueError(...)
for attr_name, attr in getmembers_static(self.__class__): | ||
# For each one, actually check if it is a property of the class and set it | ||
if isinstance(attr, JSProperty) and attr_name in kwargs: | ||
try: |
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.
Do we need this try
..? See my comment above about allow_nones
.
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.
Mmmmm... maybe? This try has been quite useful in debugging and understanding where things failed because tracebacks from the interpreter (especially mpy
) are not really helpful sometimes.
poster = JSProperty("poster") | ||
preload = JSProperty("preload") | ||
src = JSProperty("src") | ||
width = JSProperty("width") |
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.
WARNING: Untested code - for illustrative purposes only, and likely wrong (in approach and execution).
I'm wondering out loud (please ignore!) and thinking about optimising for code length (i.e. less to download), if these explicit class definitions for HTML elements could be done dynamically via re-hydrating a dict
definition of the elements:
elements = {
"a": ["download", "href", "referrerpolicy", "rel", "target", "type", ], # Attributes are always JSProperty instances?
...
}
for name, attrs in elements.items():
ElementClass = type(
name,
(TextElementBase),
{attr_name: JSProperty(attr_name) for attr_name in attrs}
)
globals()[name] = ElementClass
Like I said, not sure if this approach even works. I also have mixed feelings... explicit is better than implicit, and this may be premature optimization. Just putting it out there as an idle thought.
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.
or maybe ...
poster, preload, src, width, = (JSProperty(name) for name in ["poster", "preload", "src", "width",])
???
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.
Initially, when I started, I was creating all properties dynamically, for that exact reason. After lengthy pairing sessions and discussions with @mchilvers, we decided it wasn't worth the very few characters you save. Happy to re-open that discussion if we want to
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.
Hehehe... nope. Let's avoid that shall we, and save ourselves a lot of pain..? 🤣 If you've already trodden that path, no need to tread it again. It just wasn't apparent if you'd tried that approach already. 😉
@@ -8,7 +8,7 @@ | |||
<script type="module" src="../dist/core.js"></script> | |||
</head> | |||
<body> | |||
<script type="py" src="pydom.py"></script> | |||
<script type="mpy" src="pydom.py"></script> |
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.
🎉
Without wishing to put a spanner in the works... why doesn't this all just sit under the E.g. |
Replying for posterity. At the community call today we discussed this and, as a collective, decided to move things under the |
Description
This is a POC PR exploring a UI creation API on top of pydom
Changes
Checklist
CHANGELOG.md