-
Notifications
You must be signed in to change notification settings - Fork 514
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
feat(compat): list polyfills #6902
Conversation
@caugner Can you do a first pass of review? I would like to know if the approach I used is the right one. If so, I can go into more details (more testing!); if not, I can switch to the right one. Thank you in advance! |
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.
Overall LGTM, but some nits, and I'm not sure that it's a good idea to just append the polyfills logic to the bc-data logic.
@@ -0,0 +1,66 @@ | |||
import { DisplayH3 } from "./utils"; | |||
|
|||
const ALLOWED_POLYFILL_SERVICE = [ |
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.
Should be plural.
const ALLOWED_POLYFILL_SERVICE = [ | |
const ALLOWED_POLYFILL_SERVICES = [ |
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 You marked this as resolved, but the current status of this PR still has singular. This also applies to other threads. Did you push your changes?
const depth = $1 || 1; | ||
const queries = Array.isArray(query) ? query : [query]; | ||
|
||
var output = queries.map(query => `<div class="bc-data" data-query="${query}" data-depth="${depth}" data-multiple="${queries.length > 1}" data-polyfills="${polyfills}"> |
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.
Hm, this would mean that the same polyfills would be printed multiple times for each query. Not ideal, but that almost makes me think that we need a single separate <div class="bc-polyfills">
element instead of adding this to this div.bc-data
element.
@@ -0,0 +1,66 @@ | |||
import { DisplayH3 } from "./utils"; | |||
|
|||
const ALLOWED_POLYFILL_SERVICE = [ |
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 You marked this as resolved, but the current status of this PR still has singular. This also applies to other threads. Did you push your changes?
No I didn't push the changes yet, I still have one to make, and test locally. When ready, I'll rerequest your review. |
Alright, let me mark this PR as draft for time being, but don't hesitate to mark it ready for review again. |
This pull request has merge conflicts that must be resolved before it can be merged. |
This comment was marked as outdated.
This comment was marked as outdated.
Closing as stale, although we very much appreciate effort. 🙌 |
This is the unblocking PR for one of our Quarterly projects: we want to store polyfills only inside a YAML entry (
polyfills
) and display a sentence about them below the Compat Table.This PR:
We only authorize URLs from an allowlist to be used for this YAML header. We extend it a bit, but it should already cover the whole of web/javascript.