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

Bugfix/enable built in element extension #642

Merged
merged 37 commits into from
Nov 20, 2018

Conversation

AndyOGo
Copy link

@AndyOGo AndyOGo commented Oct 25, 2018

Fixes #607 .
Blocked by #649 #639 choojs/nanohtml#136

Changes proposed in this pull request:

  • replaced the existing polyfill with a much better, battle tested and feature complete polyfill (including built-ins support) from WebReflection
  • all kudos go to Andrea Giammarchi
  • upgrade create amo script
  • upgrades withReact to support extended builtin elements

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@@ -28,6 +27,88 @@
"include": [
"NODE_ENV"
]
}],
["babel-plugin-transform-builtin-classes", {
Copy link
Author

Choose a reason for hiding this comment

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

@AndyOGo AndyOGo changed the title WIP Bugfix/enable built in element extension Bugfix/enable built in element extension Oct 26, 2018
@LucaMele
Copy link
Contributor

Please Lets make a concrete test in this PR:

  • Lets create a extension like <table is="axa-table"></table>
  • Lets also create a patterns-library-example using that table in react.

I would merge it in this PR and when approved, make a update date and communicate it on slack. For example: "Please note, we are closing our first stable release. We realised that we cannot live anymore without supporting Custom Element Builtins. Therefore we will do Breaking change merge friday the XX.YY.KKKK".

Copy link
Contributor

@LucaMele LucaMele left a comment

Choose a reason for hiding this comment

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

for me ready to merge when a release plan is created, one component already done in the example and that component used in pattern-library-examples. We also have to communicate to all other developers to swap their polyfill

@AndyOGo AndyOGo force-pushed the bugfix/enable-built-in-element-extension branch from 4187d62 to 3d99a5d Compare November 5, 2018 07:04
@AndyOGo
Copy link
Author

AndyOGo commented Nov 5, 2018

@LucaMele
I just realized that we need to upgrade the create-amo script to support built-ins, because the syntax is very different:

  • <axa-foo></axa-foo> for custom element
    class AXAFoo extends HTMLElement {}
    customElements.define('axa-foo', AXAFoo)
  • <div is="axa-foo"></div> for extended build-in element
    class AXAFoo extends HTMLDivElement {}
    customElements.define('axa-foo', AXAFoo, { extends: 'div' })

PS:
And now we are really lucky that we abtracted our utitlities in higher order classes 🎉

@LucaMele
Copy link
Contributor

LucaMele commented Nov 5, 2018

let me know when everything is done and i will review :) also dont forget the table implementation

@LucaMele
Copy link
Contributor

LucaMele commented Nov 5, 2018

I will pay you a hot chocolate or capuccino with lots of milk for this one! Thank u man!

@AndyOGo AndyOGo force-pushed the bugfix/enable-built-in-element-extension branch from d349969 to b6a4649 Compare November 5, 2018 10:42
.eslintrc Outdated
@@ -25,11 +25,87 @@
}]
},
"globals": {
"HTMLAllCollection": true,
Copy link
Author

Choose a reason for hiding this comment

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

@LucaMele
We should double check which native constructors we really need.
I would argue that all HTML*Element constructors are sufficient.

@@ -52,8 +52,8 @@ const withReact = (WebComponent, { pure = true, passive = false } = {}) => {
// hence it's tagName could only be resolved lazily
// ref: https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry
// therefor we don't instantiate it, but rather use a statically defined tagName property
const { tagName } = WebComponent;
const displayName = `${camelize(tagName)}React`;
const { tagName, buildinTagName } = WebComponent;
Copy link
Author

Choose a reason for hiding this comment

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

Needed to support custom builtin elements with react


const mapElement = {
a: 'ATOM📗',
m: 'MOLECULE📘',
o: 'ORGANISM📙',
};

const getNativeElementConstructor = (tag) => {
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 a huge list, maybe we put it into a separate file 💣

@@ -268,29 +467,76 @@ const createBoilerplate = (_name) => {
}
};

// @todo: this should be a clean state machine
Copy link
Author

@AndyOGo AndyOGo Nov 5, 2018

Choose a reason for hiding this comment

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

This is really key, may it could be made cleaner

Copy link
Author

@AndyOGo AndyOGo left a comment

Choose a reason for hiding this comment

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

@LucaMele
I added upgrades for create AMO script and withReact

Copy link
Contributor

@LucaMele LucaMele left a comment

Choose a reason for hiding this comment

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

go for it man!

@LucaMele
Copy link
Contributor

Please communicate the breaking changes and the new API (constructor vs init)

@AndyOGo AndyOGo merged commit a1c42c2 into develop Nov 20, 2018
@AndyOGo AndyOGo deleted the bugfix/enable-built-in-element-extension branch November 20, 2018 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extending builtin elements not supported
3 participants