choojs / hyperx

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

Cannot set data-* attribute on VirtualDOM. #28

Open Bigdragon13th opened 8 years ago

Bigdragon13th commented 8 years ago

Hi,

I try to set data-* attribute to element with no luck on virtual-dom

For example: This working:

hx`<a href="${pagination.links.next}">Next</a>`

But this is not:

hx`<a href="#" data-href="${pagination.links.next}">Next</a>`

I've googled and found this https://github.com/Matt-Esch/virtual-dom/blob/master/docs/vnode.md#custom-attributes-data- which maybe related to my issue.

Bigdragon13th commented 8 years ago

For anyone who may facing the same issue, I found an easy workaround by assign data-* as a properties.attributes object after initialise a hx.

let element = hx`<a class="next" href="#">Next</a>`;
element.properties.attributes = {
    "data-href": pagination.links.next
}; 
tswaters commented 8 years ago

Interesting...

in https://github.com/substack/hyperx/pull/16 a test was added with data-* attributes, but I think the only reason this test is passing is because toString in min-document includes stray data-* attributes in the output... when returned as a dom node by a browser, (at least in Chrome, encountered this in an electron app), data attributes are missing.

There appears to be special handling for items in properties.attributes whereby they will get set via setAttribute. This would, I imagine, have the effect of setting the data-* attribute properly on the returned DOM node.

To fix this, when the tree is being populated with attributes: 1, 2 and 3; it needs to check if this is a data-* attribute and if so add it it to attributes and not on the root of the object. I can submit a PR for this.

Bigdragon13th commented 8 years ago

Thanks @tswaters for looking at this!

I've found another issue that seems related to this issue when trying to assign a "style" attribute like:

hx`<div class="box" style="background-color: red"></div>` 

This working fine in most browser except for MS Edge and Safari (didn't test on IE11 yet).

And stated in the doc: https://github.com/Matt-Esch/virtual-dom/blob/master/docs/vnode.md#propertiesstyle-vs-propertiesattributesstyle

properties.style expects an object, while properties.attributes.style expects a string.

So, to workaround this, I have to do something like:

const styleObj = {
    backgroundColor: "red"
};
hx`<div class="box" style=${styleObj}>`;
arturi commented 8 years ago

This is an issue with aria (and thus accessibility) too, Iā€™m currently struggling with it: since I have a nested tree of elements that I want to put attributes like aria-hidden on. Works fine with bel, but not virtual-dom.

Maybe we could really put this behind a supportVDomAttributes flag, as suggested in https://github.com/substack/hyperx/pull/29, or something?

return html`<div class="Uppy UppyTheme--default UppyDashboard ${isTouchDevice ? 'Uppy--isTouchDevice' : ''}"
                 aria-hidden="${modal.isHidden}"
                 aria-label="Uppy Dialog Window (Press escape to close)"
                 role="dialog">
    <div class="UppyDashboard-inner" tabindex="0">
      ...
    </div>
</div>

All attributes ā€” aria, role, tabindex ā€” except for class are missing from the resulting DOM.

tswaters commented 8 years ago

Sorry for all the noise there, pushed up some force commits after I noticed some errors...

The PR has been adjusted to use a flag, {vdom: true} - this will fix data-, aria- and style being passed as a string. I noticed that tabindex and role both seemed to work without any explicit modifications, so maybe that was a bug upstream that is now fixed.

That reminds me - on the note of upstream bugs. Note that the tests relating to style as a string won't pass unless min-document v2.18.1 is present, due to this bug. This matches the version in this package.json so it should be fine.