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

Feature/add custom element built in support #137

Merged

Conversation

AndyOGo
Copy link
Contributor

@AndyOGo AndyOGo commented Nov 6, 2018

fixes #136

  • adds template literal support for extended built-in elements (custom elements V1 spec)
  • browserify and babel transform upgrades

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 6, 2018

@goto-bus-stop
This would still need some tests, though for these we would need some additional requirements like polyfills for custom elements V1
I decided to just stub document.createElement for this, hence testing custom elements themselves is out of scope of this project.

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 6, 2018

@goto-bus-stop
@bcomnes
@shama
@yoshuawuyts
I have added tests:)
May I asked you to review it please?

var optionsArg

// this iife is a must to avoid illegal invocation type errors, caused by transformed nanohtml tests
(function () {
Copy link
Contributor Author

@AndyOGo AndyOGo Nov 6, 2018

Choose a reason for hiding this comment

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

If tape would have before/after or setup/teardown support it could be implemented cleaner, but so far it gets the job done

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 8, 2018

@goto-bus-stop
@bcomnes
@yoshuawuyts
Hope you have some free time to look into this.
I'm looking forward for your feedback

@bcomnes
Copy link
Collaborator

bcomnes commented Nov 8, 2018

I would guess @goto-bus-stop is the most qualified to review this. I do know if we extend nanohtml we may also need to extend/test yoyoify to support the new changes.

@goto-bus-stop
Copy link
Member

maybe yo-yoify should be deprecated on npm, this module includes a browserify transform

i had a quick glance earlier and the approach looks good. i can prob review this proper on the weekend

@bcomnes
Copy link
Collaborator

bcomnes commented Nov 8, 2018

Oh yeahhhh. I forgot about that. +1 to that

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 8, 2018

@bcomnes
@goto-bus-stop
Awesome guys, thanks for your feedback and this great library.
Take your time to review it unhurriedly, I'm looking forward for your review:)

} else if (tag === COMMENT_TAG) {
return document.createComment(props.comment)
} else if (isCustomElement) {
el = document.createElement(tag, { is: isCustomElement })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Create the element
if (ns) {
el = document.createElementNS(ns, tag)
if (isCustomElement) {
el = document.createElementNS(ns, tag, { is: isCustomElement })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 13, 2018

Hi @goto-bus-stop
hmh, I guess you have lots of stuff to do?
Unfortunately I have a giant refactoring waiting to be reviewed and merged but it relies upon this PR.
I hope you are happy with it and I would be super lucky if you find some time soon to check it 🙏

Copy link
Member

@goto-bus-stop goto-bus-stop left a comment

Choose a reason for hiding this comment

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

looks great. one test nit & then it's good to go!

tests/browser/elements.js Show resolved Hide resolved
@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 15, 2018

Awesome:) I have a little question too:)

@goto-bus-stop goto-bus-stop merged commit 4aa2ad9 into choojs:master Nov 16, 2018
@AndyOGo AndyOGo deleted the feature/add-custom-element-built-in-support branch November 16, 2018 12:31
@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 16, 2018

@goto-bus-stop
Great. Thanks for merging and releasing 1.3.0 🚀
Unfortunately the latest version at npm I can install is 1.2.6, May I ask if you published already?

@goto-bus-stop
Copy link
Member

goto-bus-stop commented Nov 16, 2018 via email

@AndyOGo
Copy link
Contributor Author

AndyOGo commented Nov 16, 2018

Awesome, thank you so much :)
Just installed it:)

And it works great.

I'm doing a big refactoring which extends built-in elements.

Btw. I read in your release notes that autonomous custom elements <custom-element> aren't still supported. I wonder how you mean it, because I use them heavily since months 🤔

@goto-bus-stop
Copy link
Member

lol, you're right. i was thinking of #121 but that's actually not about <custom-element>, but about passing in custom element classes as objects <${CustomElement} />. updated the release note, thanks for the correction! 😆

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.

Support custom elements V1 extension of built-in elements
3 participants