hyperx icon indicating copy to clipboard operation
hyperx copied to clipboard

fix some attributes not being set properly under vdom

Open tswaters opened this issue 7 years ago • 6 comments

fixes #28

I tried to keep with the coding convention - hope this is OK.

also, I wasn't able to run the coverage script on windows without manually expanding the glob pattern :( ... there's a few failures when running that, but it's nothing that has been introduced here.

tswaters avatar Jul 09 '16 03:07 tswaters

+            if (/data-/.test(key)) data(cur[1], key, parts[i][1])

Should that be /^data-/?

I'm not sure that other implementations aside from virtual-dom use an attributes property. If enough of them do, this seems ok but otherwise it would be best to put this behavior behind a flag or to have a separate module somewhere to wrap data attributes:

var h = require('virtual-dom/h')
var hx = require('hyperx')(function (tagName, props, children) {
  if (!props.attributes) props.attributes = {}
  Object.keys(props).forEach(function (key) {
     if (/^data-/.test(key)) props.attributes[key] = props[key]
  })
  return h(tagName, attr, children)
})

ghost avatar Jul 25 '16 05:07 ghost

Should that be /^data-/ ?

Yes... yes it should.

This approach in general is a good call. I found that when the change applied to all engines, hypertext promptly blew up. (seems it doesn't like attributes being a object there; it tries to call forEach on it)

Anyway, I've added the flag as {vdom: true} and updated the fix to apply to all the data-, aria- attributes, and when a style is provided as a string instead of an object. All of these should go under props.attributes

tswaters avatar Sep 03 '16 02:09 tswaters

Any update on this?

Bigdragon13th avatar Nov 03 '17 08:11 Bigdragon13th

I don't think hyperx should add logic specific to a particular virtual dom library. I think substack's custom factory suggestion is a better approach.

goto-bus-stop avatar Mar 23 '18 11:03 goto-bus-stop

fwiw, that's basically what the PR does, but it puts it behind a flag so the library can handle it for users in a consistent manner.

If consumers do need to implement this in their own app, a note should probably be added to the readme about lack of aria-, data- and style attributes when using virtual-dom.... add a sample implementation, and maybe a link to the vnode docs in virtual-dom (https://github.com/Matt-Esch/virtual-dom/blob/v2.1.1/docs/vnode.md#propertiesstyle-vs-propertiesattributesstyle)

For posterity, an implementation that supports data-, aria- and style attributes:

var h = require('virtual-dom/h')
var hx = require('hyperx')(function (tagName, props, children) {
  if (!props.attributes) props.attributes = {}
  Object.keys(props).forEach(function (key) {
    if (/^(data|aria)-/.test(key) || (key === 'style' && typeof props[key] === 'string')) {
      props.attributes[key] = props[key]
    }
  })
  return h(tagName, attr, children)
})

tswaters avatar Mar 27 '18 01:03 tswaters

I think a note like that in the virtual-dom example section would be great, yes :)

goto-bus-stop avatar Mar 28 '18 09:03 goto-bus-stop