Skip to content
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

fix some attributes not being set properly under vdom #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tswaters
Copy link

@tswaters tswaters commented Jul 9, 2016

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.

@ghost
Copy link

ghost commented Jul 25, 2016

+            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)
})

@tswaters
Copy link
Author

tswaters commented Sep 3, 2016

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 tswaters force-pushed the issue-28 branch 3 times, most recently from 238830d to 20e817d Compare September 3, 2016 02:54
- enabled by passing {vdom: true} flag to the hyperx contructor options

- fixes data-*, aria-* and style as a string.  vdom expects these to be
  under an 'attributes' property under props and will gladly ignore them
  otherwise

fixes choojs#28
@tswaters tswaters changed the title fix data-* attributes not being set properly fix some attributes not being set properly under vdom Sep 3, 2016
Copy link

@brokenalarms brokenalarms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was looking for a fix for exactly this (switching view framework from React to something else breaks as styles need to be specified as an object in one, and a string in another), so it would be great if this could be merged.

EDIT - sorry I was only referring to the styles fix, not moving anything into attributes object. What would be best/most generic would be the ability to pass opts.wrapper to hyperx, which then could apply additional wrapping to h before returning the result to hyperx.

@Bigdragon13th
Copy link

Any update on this?

@goto-bus-stop
Copy link
Member

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.

@tswaters
Copy link
Author

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)
})

@goto-bus-stop
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot set data-* attribute on VirtualDOM.
4 participants