-
Notifications
You must be signed in to change notification settings - Fork 129
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 more video attributes #379
base: main
Are you sure you want to change the base?
Conversation
Great question! I have been thinking and dealing with this for the last couple of weeks. What you want is to clone the whole template. That's for attributes/properties to be inlined. Think of the following: const template = document.createElement('template')
template.innerHTML = '<div class="something"/>' that's faster than const template = document.createElement('template')
template.innerHTML = '<div/>'
template.firstChild.className = 'something' The templates, are usually bigger than this, which makes it even more desirable. But the idea has been illustrated. (much faster to clone a chunk of html, than to clone it and then having to set some properties on it) So this means you would want the lowercase and dashes spaced version whenever possible(attributes), so the template can be inlined and cloned faster. This is desired because of two reasons: template cloning and ssr. The only reason why you may want a camel Case version is when the value its dynamic (it cannot be inlined because the value is not known), and if the user used a camel case then it's a property. Solid has so many optimizations, that It's hard to follow. Basically it uses Glad to see you here! |
LGTM your PR! |
Thank you @titoBouzout! It makes sense wanting to inline as much as possible into the template HTML string. And according to MDN the HTML attributes are indeed all lowercase. But as far as the types goes, it seems like allowing the properties to be added with camelCase (which I think is slightly easier to read) still yields the same output, so it seems like the compiler treats it all the same in the end? See the output tab here: https://playground.solidjs.com/anonymous/aea47fbf-a24d-4260-adad-8a5bbc342b22 Somewhat related… why is |
I am trying to get rid of any attribute/property transformation, its just problematic. For example on this case you are just lowercasing
There's a Map that holds a couple of attributes that are locked to be handled as properties, for different reasons I suppose. I also would like to get rid of this hardcoded properties and lookup for setters instead. https://github.com/ryansolid/dom-expressions/blob/main/packages/dom-expressions/src/constants.js |
Wait does @titoBouzout I don't think it is possible realistically to get rid of all the looks up. But for the most part I think that JSX should reflect the attributes has been my thinking. Although stuff like this is questionable. As now it looks like we have properties that aren't attributes. |
Added some more of these attributes that were lacking https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video
I added them in all lower case but as a user I'd prefer the camel cased version.
I can see some attributes are both all lower case and camelCase like here: https://github.com/danieltroger/dom-expressions/blob/0d5c46b24c9fba2d62dee8bc1acbf98c733fa633/packages/dom-expressions/src/jsx.d.ts#L1173-L1174
What's solid-js' convention?