add class and style interfaces #193
Conversation
There was a problem hiding this comment.
I've added a few comments.
Since I've generally also experimented on this, this can maybe a source of inspiration as well.
When I remember right, I stopped experimenting with this, because of the added complexity (and resulting code-size). Where I'd think that a more static way (as described in the comment with the assoc state bounds) is more future-proof and less bloaty/easier to strip/optimize by the compiler while being easier to reason as well probably. But this is speculative.
crates/xilem_web/src/context.rs
Outdated
| pub(crate) current_element_attributes: VecMap<CowStr, AttributeValue>, | ||
| pub(crate) current_element_classes: VecMap<CowStr, ()>, | ||
| pub(crate) current_element_styles: VecMap<CowStr, CowStr>, |
There was a problem hiding this comment.
Not that it is particularly relevant to this PR, more a brain-dump/thoughts about it, I'd like to share.
I'm more and more thinking about getting the temporary state of elements out of the context again, likely via associated type bounds as described here, since they're stabilized now. This would allow (safe) implementations downstream (i.e. remove the "need" for the Sealed trait in the interfaces), better separation of concerns (especially if there's a lot of logic added for all kinds of dom elements), possibly it would also help stripping down to only relevant code (e.g. when style is not used, it could be stripped away in the resulting binary).
And generally make everything cleaner I think. I think it's probably worth the added compilation times. But this probably needs deeper investigation/experimentation (again).
|
@Philipp-M Thanks for the review, and for linking to the great conversations that I missed on zulip. I'll do another draft based on the comments when I get chance. |
|
Hi @Philipp-M I've made some of the changes you suggested, and in particular went in ot the trait-based polymorphism approach. Let me know what your thoughts are. |
|
|
||
| /// A trait to make the class adding functions generic over collection type | ||
| pub trait IntoStyles { | ||
| fn into_styles(self, styles: &mut Vec<(Cow<'static, str>, Cow<'static, str>)>); |
There was a problem hiding this comment.
I like this approach 👍
I think wasm binary size should stay compact that way, and it's possible to add modifier style types.
Not relevant to that PR (some thoughts):
We could add some kind of flag (enum?), whether e.g. a style attribute should be able to be overwritten for each entry, or in what order etc..
| let mut styles = vec![]; | ||
| style.into_styles(&mut styles); | ||
| Style { | ||
| element: self, | ||
| styles, | ||
| phantom: PhantomData, | ||
| } |
There was a problem hiding this comment.
Maybe something to explore (possibly in a future PR):
keeping the styles attribute generic as impl IntoStyles and put it into Style directly (as Style<impl IntoStyles>).
This would require IntoStyles to act on the reference instead of moving (so a different name like AsStyles makes sense maybe?), but I think this could save a few allocations, as it could be done on the fly while View::build/View::rebuild, at least if the type itself doesn't require an additional allocation (such as String)), and more importantly: It could also enable modifying styles of the element inplace with different views (via wrapping types etc.).
But it's possibly also a tradeoff in wasm-binary size (on the other hand, it's already parameterized anyway over the wrapped element E and T and A, so probably not necessarily a regression in that regard)
There was a problem hiding this comment.
I think there are opportunities to save allocations, but I suggest we push that to a separate PR.
Co-authored-by: Philipp Mildenberger <[email protected]>
Co-authored-by: Philipp Mildenberger <[email protected]>
Co-authored-by: Philipp Mildenberger <[email protected]>
Co-authored-by: Philipp Mildenberger <[email protected]>
There was a problem hiding this comment.
Looks good, thanks. Just this comment (was probably missed):
... so to be completely accurate (as e.g. MathML AFAIK doesn't support style) it would make sense to put
Element::styleinHtmlElement::styleandSvgElement::style(methods { fn style(..) {..} } in dom_interface_macro_and_trait_definitions)
But I can do that in a follow-up PR too and it's not super important
I'm not 100% sure I know exactly what you're asking for. Perhaps you could do this in a follow-up PR? :) |
This PR adds more typed interfaces for adding classes and styles, using the DOM APIs under the hood.
I'm opening the PR for discussion. If it's accepted, I'd like to do some tidying up and adding tests before merging.