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: parse classnames both as string (with tagname), and as "class" attribute #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

laggingreflex
Copy link

@laggingreflex laggingreflex commented Feb 24, 2017

Fixes when you have a usecase like this (which didn't work)

h('div.one', {class: {two: true}}, [...])`

It made the class [object Object] one

This fixes it.

Fixes queckezz/preact-hyperscript#3

…ttribute

Fixes when you have a usecase like this (which didn't work)

```
h('div.one', {class: {two: true}}, [...])`
```

It made the class `one [object Object]`

This fixes it.
classnames defined in tha tagname should come before the ones defiend in "class" attributes.
Object.keys(attrs.class).forEach(existing => {
newClassesObj[existing] = attrs.class[existing]
})
attrs.class = newClassesObj
Copy link
Author

Choose a reason for hiding this comment

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

This is to preserve order. Classnames defined with tagname should come first (IMO).

@queckezz
Copy link
Owner

queckezz commented Mar 2, 2017

Awesome! Can you add a test for it?

@laggingreflex
Copy link
Author

added tests f97194c

test/index.js Outdated
const obj = parse(['p.italic', { class: 'bold' }])
t.equal(obj.attrs.class, 'italic bold')
t.end()
})

test('parse selector class with attributes class as array', (t) => {
const obj = parse(['p.italic', { class: ['bold'] }])
t.deepEqual(obj.attrs.class, ['italic', 'bold'])
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand how obj.attrs.class would be anything else than a string or an object.

based on the definitions, no array support is included in preact

https://github.com/developit/preact/blob/523c397a8c99560fea77ef558c04beb27c99eb7c/src/preact.d.ts#L281-L282

Copy link
Author

Choose a reason for hiding this comment

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

preact-hyperscript supports it (it passes it through classNames) https://github.com/queckezz/preact-hyperscript/blob/master/src/index.js#L30

Copy link
Author

Choose a reason for hiding this comment

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

I guess I see now when you originally said that this could be a lot of extra code. If you don't want this to be in parse-hyperscript, and rather this should only be done in preact-hyperscript if it's the one that supports classNames then for this very specific use case (classname as array) it might require doing the work of this PR in preact-hyperscript itself (and even then probably here as well). Ah well, let me know how you wanna proceed

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.

Classnames both as string (with tagname), and as "class" attribute
2 participants