feat(h): make h set attributes on custom-element via attributes prop #15
Conversation
There was a problem hiding this comment.
I'd like to see if we can make the h behaviour same to what's being proposed in facebook/react#7249 as well as what's in skatejs/dom-diff and also being proposed in google/incremental-dom#316.
Additionally, I think the test can be a lot simpler for this use case.
test/unit.js
Outdated
| @@ -26,7 +26,62 @@ describe('bore', () => { | |||
| expect(<Fn />.localName).to.equal('div'); | |||
| }); | |||
|
|
|||
| it('setting attributes', () => { | |||
| it('setting attributes on CustomElement', () => { | |||
There was a problem hiding this comment.
You shouldn't need to test a custom element here. It's all just DOM. You should just be able to test a <div /> with something like:
const el = mount(<div prop={true} attributes={{ test: 'test' }} />);
expect(el.prop).to.equal(true);
expect(el.hasAttribute('prop')).to.equal(false);
expect(el.attr).to.equal(undefined);
expect(el.getAttribute('attr')).to.equal('test');There was a problem hiding this comment.
right, actually that is in next test. totally missed that ( also because non obvious naming of attrs test1 and so on :P )
There was a problem hiding this comment.
also because non obvious naming of attrs test1 and so on
Not sure if I understand that statement.
If it's already being tested, it'd be good if we can just remove this test. It's quite complex and difficult to understand.
There was a problem hiding this comment.
that numbers suffix confused me I guess, so I didn't pay close attention to them, but my bad. sry nevermind
src/index.js
Outdated
| } | ||
|
|
||
| function handleFunction (Fn) { | ||
| return Fn.prototype instanceof HTMLElement ? new Fn() : Fn(); | ||
| } | ||
|
|
||
| function setAttr (node, attrName, attrValue) { |
There was a problem hiding this comment.
I think it might be good to just make this a breaking change and require attributes to set attributes (or attrs?). Everything else just sets props. Maybe at the same time we can do events? I'd like to split this out into a separate library for declaratively constructing real DOM eventually, also.
There was a problem hiding this comment.
There was a problem hiding this comment.
I think it might be good to just make this a breaking change
why BC? it's clearly a feature. Currently there is no option to set attributes via bore within test, only via strings ( which doesnt work either #12 )
Overall as I re-thought the approach, overriding attributes feels just wrong ( although google/incremental-dom#316 is going to use it ) . I would rather go as you've suggested with attrs ( also because from Typescript perspective, native HTLMElement.attributes cannot be overriden via just object ( because under the hood it's not, it's NamedNodeMap ) - with attrs smooth typechecking can be provided )
Maybe at the same time we can do events?
I agree, but that should be addressed in next PR
There was a problem hiding this comment.
It's calling node.setAttribute() if attributes is not defined. What I'm saying is that it should be setting a prop if no attributes is defined.
I agree, but that should be addressed in next PR
Sure, but it'd be nice if it could happen soon after.
There was a problem hiding this comment.
It's calling node.setAttribute() if attributes is not defined. What I'm saying is that it should be setting a prop if no attributes is defined.
Oh right. But I like your idea https://github.com/skatejs/dom-diff#hname-props-childrenortext which may not be consistent with future changes within iDom.
So ok, let's make it BC then
There was a problem hiding this comment.
So as I recap.
I vote for attrs only for setting attributes. events for setting events.
Pls lemme know your decision so I can implement it, and merge ASAP.
thanks!
BREAKING CHANGE:
Before you could set attributes via bore's h via data-* or aria-* otherwise the attribute would be set to element property.
Now if you wanna set attributes you have to explicitly do it via `attrs` property which accepts a object map.
Before:
```js
<my-element aria-role="button" data-foo="hello" />
```
After:
```js
<my-element attrs={{ 'aria-role': 'button', 'data-foo': 'hello' }} />
```
|
changes added. re - review pls @treshugart If LGTM I will update the docs as well as part of this PR, then we can merge |
src/index.js
Outdated
| } | ||
|
|
||
| function handleFunction (Fn) { | ||
| return Fn.prototype instanceof HTMLElement ? new Fn() : Fn(); | ||
| } | ||
|
|
||
| function setAttrs (node, attrValue) { | ||
| Object.keys(attrValue) | ||
| .forEach((key) => { node.setAttribute(key, attrValue[key]); }); |
|
Hey @Hotell instead of adding nits / codestyle stuff (that semistandard didn't seem to catch) I've just created a PR for some minor things rather than going back and forth or pushing directly to your branch. Cheers! |
|
Approved pending other PR is either merged here, or this is merged and that is retargeted to master. |
attrs)eventsCloses #14