| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
On the Web, append(...items) accepts nodes and strings (or not even strings, anything that is not an element) but it uses document.createTextNode(item_value) instead, it doesn't create tons of <span> nodes. Adding elements instead of just text can cause issues with both CSS and layout performance, because elements are reachable via CSS while raw text nodes aren't. I hence believe this is a great draft for some improvement, and I'd like to thank you for that, but I think we should have a pyscript.web.text and use it but, if we don't, we should take that opportunity. It's _dom_element as class should return a text node, not a container like a span is, then we can improve append ... what do you think? /cc @ntoll |
Sorry, something went wrong.
|
Hey hey @iliketocode2 - good first draft. I agree with @WebReflection that adding elements rather than just a pure text node (see: https://developer.mozilla.org/en-US/docs/Web/API/Text) could create more complications. This is very much a case of finding the simplest most idiomatic solution - and that takes time and exploration (which is what's happening here - so you're doing exactly the right thing, and we're demonstrating why we have PRs for review!). I imagine (and I AM imagining - I've not checked), you could just do something like: text_node = document.createTextNode(item) self._dom_element.appendChild(text_node) But appendChild may just accept the item and convert the string automatically into a text node (I'm not sure - check). Take a look and let us know how you get on! 👍 |
Sorry, something went wrong.
|
@iliketocode2 if you're not doing anything at the time of our community technical call today, you could add discussion of this PR to our agenda? We could discuss the approach and any findings you have in the call. 👍 |
Sorry, something went wrong.
|
Thank you both for your feedback! Understood, it makes sense that this implementation could cause issues with applying CSS properties and also general HTML disruptions, thanks for pointing that out! @ntoll I tried the logic that you suggested (that was my first go-to based on the structure of the other cases in .append) but for some reason my test suite keeps failing with TypeError: Cannot append str to element (I wonder if perhaps web.py is not updating every time I run the test suite with npm run build?). The same TypeError occured with various methods involving .createTextNode which made me think perhaps there was an issue that required html injection via the span element, hence my above implementation. The TypError would persist even if casting item as str in web.py. I can't make it to the community call today due to class :( but I'll play around some more today and tomorrow and also verify that I'm running the tests on an updated web.py... Thanks again for both of your feeback! |
Sorry, something went wrong.
|
@iliketocode2 I think all this MR requires is another elif and literally nothing else: # ...
if isinstance(item, Element):
self._dom_element.appendChild(item._dom_element)
elif isinstance(item, ElementCollection):
for element in item:
self._dom_element.appendChild(element._dom_element)
elif isinstance(item, (list, tuple)):
for child in item:
self.append(child)
# HERE 👇
elif isinstance(item, (str, int, float, bool)):
self._dom_element.append(item)
# ...after all, the DOM has a native append API that does the right thing so why shouldn't we use it? it's as convenient as any other DOM API and it works beautifully well 'cause it's "native" for the operation. there's not even any need to create a text node, the API does that for us ... deal? |
Sorry, something went wrong.
|
Wonderful, thank you @WebReflection! I agree, using the native DOM append is certainly the simplest and most 'native' solution as you suggest. I realize now why all of my previous tests with similar code have been failing -- I missed this test at the very bottom of the test suite that was enforcing the old behavior. def test_invalid_append_type(self):
"""Test that appending invalid types raises TypeError."""
div = web.div()
with upytest.raises(TypeError):
div.append(12345) # Numbers can't be appended
I'll make the appropriate changes and revise this PR. Thanks again! |
Sorry, something went wrong.
There was a problem hiding this comment.
CI is green, the added code is minimal and explicit, tests have been improved, docs updated ... what's not to like about this MR? 🥳
Thank You 🙏
if you cannot merge yourself, I am going to do this without regrets 👋
Sorry, something went wrong.
|
Bravo @iliketocode2 - you're now an open source contributor. We should be releasing the next version of PyScript today. 🎉 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Description
Resolving Issue #2418
This PR updates the Element.append method to support strings. Previously, attempting to append a string would raise a TypeError (can be seen in this PyScript app created by jolaf). Now, strings are automatically wrapped in a span element, allowing them to be appended to dom elements.
Changes
Checklist