search

choojs / hyperx

🏷 - tagged template string virtual dom builder
BSD 2-Clause "Simplified" License
1.01k stars 48 forks source link

Don't change attributes to properties by default. #50

Closed jorgebucaran closed 6 years ago

jorgebucaran commented 7 years ago

Hi👋, the idea with this PR is to make hyperx behavior less opinionated.

Also related: #48, #49.

yoshuawuyts commented 7 years ago

@jbucaran could you perhaps explain a little what the intent is here? - I'm not sure I quite understand yet why this behavior change would be needed

jorgebucaran commented 7 years ago

@yoshuawuyts Of course! It's related to https://github.com/substack/hyperx/pull/49 where a second argument, options, was introduced to opt out attrToProp.

The default behavior in hyperx is to chage props in the data argument like class to className.

This behavior is not hyperx's concern and should be left to DOM engines to deal with it, instead of having the parser/hyperx deal with it in an arbitrary way.

jorgebucaran commented 7 years ago

@substack Hi there, can you review this or provide some resolution / advice?

Rob-pw commented 7 years ago

I'm going in circles here, is this issue relating to attribute class being renamed to className? It's my first experience of hyperx/hyperapp and this one quirk alone is rather confusing.

jorgebucaran commented 7 years ago

@Rob-pw

The current behavior here is to change some of the props that you pass in the data argument, e.g., class is changed to className.

This is inconvenient for v/DOM authors that want to support hyperx, like I do with hyperapp, because it forces my code to check if an element's attribute is className and then rename it to class for no good reason.

Rob-pw commented 7 years ago

Is there a good solution for this bar running off with jsx? At the moment it seems broken, attribute class gets changed to classname (lowercase, and is left like that on the element, meaning no styles are applied). I've tried using class , className, classname, in my markup but all of these end up as classname on the element. I was wondering why none of the examples use a class attribute.

<button class="mdl-button">
  Button
</button>
Rob-pw commented 7 years ago

Ah yes, the solution seems to be in the PR that I am commenting on. Thanks @jbucaran. If this could be merged into master, that'd be ace.

jorgebucaran commented 7 years ago

@substack Is this going to get merged?

jorgebucaran commented 6 years ago

@yoshuawuyts Could you have a look?

yoshuawuyts commented 6 years ago

Haha, hey! — sorry for not getting back to you sooner. I think the tradeoff here is that it'll probably break what we're doing with Choo and other modules, which simply means spending some time to make sure everything still works.

I'm a bit constrained on time at the moment, hence why I've kinda just left this; and I guess nobody else has picked this up either.

I'll be in Tokyo in a few weeks; if you want we can perhaps hang out sometime, merge this, and fix the fallout together C: It's all just chores, so not being alone in doing this would be neat haha. Cheers!

jorgebucaran commented 6 years ago

@yoshuawuyts That would be great. Oh, really, are you coming to nodefest perhaps?

yoshuawuyts commented 6 years ago

@JorgeBucaran December 1st; so right the week after :D