-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Editorial: Add CustomElementRegistry platform support #623
Conversation
If this looks like i'm going in the right direction I can implement the following nested methods:
Also noticed broken build. Please advise |
- [x] √ CustomElementRegistry` Basic support - [ ] √ CustomElementRegistry.define()` support - [ ] √ CustomElementRegistry.get()` get - [ ] √ CustomElementRegistry.whenDefined()` whenDefined Addresses: - WICG/webcomponents#664 Related MDN Documentation: - `CustomElementRegistry.define()` - https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define - `CustomElementRegistry.get()` - https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/get - `CustomElementRegistry.whenDefined()` - https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/whenDefined References: - https://caniuse.com/#feat=custom-elementsv1 - Chrome Status - http://www.chromestatus.com/feature/4696261944934400 - Firefox Status - https://platform-status.mozilla.org/#custom-elements - Webkit Status - http://webkit.org/status.html#feature-custom-elements - Edge Status - http://status.modern.ie/customelements Notes: - Firefox flag status - https://bugzilla.mozilla.org/show_bug.cgi?id=889230#c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is overall quite good. Most of the comments are the usual suspects when doing it for the first time.
api/CustomElementRegistry.json
Outdated
"support": { | ||
"chrome": { | ||
"version_added": "33", | ||
"notes": "Chrome 36+/Opera 20+ implemented a previous version of Custom Elements (v0) that used `.registerElement()`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opera 20+ has nothing to do here, it is an entry about Chrome.
Chrome 36+ is unclear, we write it clear "From Chrome 36, …" (formulation can be done a bit differently).
Also it looks like the note is a restriction/limitation that has been added after the initial release (33). Are you sure of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes a bit confusing going back and forth between caniuse. I feel there are some misleading statements. Perhaps need to address there. Looks like 54 is v1 true version.
Fixed in 85a11a3
api/CustomElementRegistry.json
Outdated
"version_added": "41", | ||
"notes": "Supports 'Autonomous custom elements' but not 'Customized built-in elements'" | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No empty line please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in: c9714c4
api/CustomElementRegistry.json
Outdated
"notes": "Supports 'Autonomous custom elements' but not 'Customized built-in elements'" | ||
}, | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in: c9714c4
api/CustomElementRegistry.json
Outdated
}, | ||
|
||
|
||
"chrome_android": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put entries in this order:
webview_android, chrome, chrome_android, edge, edge_mobile, firefox, firefox_android, ie, ie_mobile, opera, opera_android, safari, safari_ios.
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm out of curiosity why webview first @teoli2003? Also firefox_android has no specs as far as caniuse goes. For future reference do we totally remove or put null?
Fixed in: f17a21a
Also took the liberty to update documentation #625 as per your recommendation.
api/CustomElementRegistry.json
Outdated
|
||
"chrome_android": { | ||
"version_added": "61", | ||
"notes": "Supports 'Autonomous custom elements' but not 'Customized built-in elements'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first look, I think customized built-in elements should be a subfeature here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoli2003 not sure I follow. I merely used GainNode
as a reference point so not sure I know how to do this as this was not in there. Any examples?
api/CustomElementRegistry.json
Outdated
}, | ||
"safari": { | ||
"version_added": "10.1", | ||
"notes": "Supports 'Autonomous custom elements' but not 'Customized built-in elements'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first look, I think customized built-in elements should be a subfeature here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoli2003 are we certain? Because the implementors (vendors) have no immediate plan to support this (with the exception of Chrome).
Please advise and I will update accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoli2003 also would .define()
, .get()
, & .whenDefined()
be considered "Sub-Features"?
api/CustomElementRegistry.json
Outdated
"CustomElementRegistry": { | ||
"__compat": { | ||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/CustomElementRegistry", | ||
"description": "The CustomElementRegistry interface provides methods for registering custom elements and querying registered elements.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually don't add a description at the interface level (it is more for subfeature that are neither interfaces, methods nor properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5953258
api/CustomElementRegistry.json
Outdated
"support": { | ||
"chrome": { | ||
"version_added": "33", | ||
"notes": "Chrome 36+/Opera 20+ implemented a previous version of Custom Elements (v0) that used `.registerElement()`" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"chrome_android" has an additional restriction note, as well as safari. Doesn't it apply to "chrome" too? (And Opera): if so, they have to be added to each fo them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoli2003 how do we do multiple notes...an array? Perhaps i need to RTFM more hehe. ;-)
Fixed in: f8186a5
api/CustomElementRegistry.json
Outdated
"version_added": false | ||
}, | ||
"opera_android": { | ||
"version_added": false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure Opera restrict this to the desktop version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoli2003 addressed all of your concerns. Besides a couple of nits this patch should be more to your liking. |
@teoli2003 just came across |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit and I think this should be good.
api/CustomElementRegistry.json
Outdated
}, | ||
"firefox": { | ||
"version_added": true, | ||
"notes": "Enabled through the `dom.webcomponents.enabled` preference in `about:config`", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note is not needed, the MDN compat tables generate a consistent note from the flag object which we can also localize in the future. Please remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, the generated note will be:
"This feature is behind the dom.webcomponents.enabled
preference (needs to be set to true
). To change preferences in Firefox, visit about:config."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Elchi3 pardon my inexperience this schema is super confusing at least to me. Why would that not be a note of the flags section?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The flag section contains all the information about the pref to enable. There is no need to write the note explicitely. From the flag section information, macros can generate the note synthetically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Great explanation @teoli2003.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teoli2003 Fixed in @teoli2003 Fixed in 0b27628
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@teoli2003 @Elchi3 next steps? Should we merge this or shall I complete Please advise... |
@snuggs I propose that you complete the three missing and we do a final review before merging all of them. The next package published will be next Monday, so we don't need to rush here. |
Fantastic @teoli2003. Also is there a quick briefing how these get moved over to MDN? I started noticing updates to COMPAT tables recently but can't seem to find a tutorial on the code references when editing MDN docs. Any help would be appreciated. |
There is an MDN help page available about this: https://developer.mozilla.org/en-US/docs/MDN/Contribute/Structures/Compatibility_tables |
I think we should merge this. We're planning to slightly change the schema for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lGTM, r+.
Thanks a lot!
Yes, yolu are right. Let's merge this. |
Awesome! Feels great! Looking forward to more @teoli2003 |
@teoli2003 @Elchi3 just realized I need to complete the following APIs:
How are we doing on the new schema update? I figured if given some time over holidays things would be a little more stable. Please advise on how I can wrap this up. 🙏 |
Addresses:
- WICG/webcomponents#664
Related MDN Documentation:
-
CustomElementRegistry.define()
- https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/define-
CustomElementRegistry.get()
- https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/get-
CustomElementRegistry.whenDefined()
- https://developer.mozilla.org/en-US/docs/Web/API/CustomElementRegistry/whenDefinedReferences:
- Can I Use - https://caniuse.com/#feat=custom-elementsv1
- Chrome Status - http://www.chromestatus.com/feature/4696261944934400
- Firefox Status - https://platform-status.mozilla.org/#custom-elements
- Webkit Status - http://webkit.org/status.html#feature-custom-elements
- Edge Status - http://status.modern.ie/customelements
Notes:
- Firefox flag status - https://bugzilla.mozilla.org/show_bug.cgi?id=889230#c9