-
Notifications
You must be signed in to change notification settings - Fork 515
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
Support giving spec URL as {{Specifications(...)}} argument #5329
Support giving spec URL as {{Specifications(...)}} argument #5329
Conversation
Related: mdn/content#13273 (comment) This change allows authors to give the Specifications macro a spec URL as an argument; like this: {{Specifications("https://w3c.github.io/webappsec-secure-contexts/")}}
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, I'm not quite sure what the changes in document-extractor.js
do, so I added some remarks.
let browsers; | ||
let data; |
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.
Where are browsers
and data
used, apart from their assignment?
@@ -342,8 +342,17 @@ function _addSingleSpecialSection($) { | |||
const [proseSections] = _addSectionProse($); | |||
return proseSections; | |||
} | |||
const query = dataQuery.replace(/^bcd:/, ""); | |||
const { browsers, data } = packageBCD(query); | |||
let query = dataQuery.replace(/^bcd:/, ""); |
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 looks like this variable is only read once in the else
branch.
Could you move it there and make it const
?
let browsers; | ||
let data; | ||
|
||
if (dataQuery && dataQuery.trim().match("^http[s]?://")) { |
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.
Could you add a short comment before this block explaining what it does / we need this?
@sideshowbarker Please see @caugner’s comments when you have a moment. Thank you. |
I’m closing this PR in favor of consideration for #5971 and #5972, which essentially would allow us to move completely away from ever using the The context is that #5347 (reply in thread) has persuaded me that it would be a good idea for us to move wholly over to just using the The specific parts of #5347 (reply in thread) I found persuasive were:
So #5971 and #5972 are also in line with our general de-macro-ization plans — in that they’d take us a big step closer to completely eliminating the |
Related: mdn/content#13273 (comment) • This changes allows authors to give the Specifications macro a spec URL as an argument; like this:
cc @wbamberg