-
Notifications
You must be signed in to change notification settings - Fork 518
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
Add support for 'spec-urls' frontmatter key #5284
Add support for 'spec-urls' frontmatter key #5284
Conversation
Fixes #4704 Related: #5277 This change adds support for listing specs in the Specifications section based on the value of the 'spec-urls' frontmatter key — instead of or in addition to listing specs based on the value of the 'browser-compat' frontmatter key, or the value of the parameter passed to the Specifications macro.
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.
Nice work, Mike! Looks good to me 👍
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, thanks @sideshowbarker 🎉
@schalkneethling @sideshowbarker How does this work?
|
I’ve started writing some documentation for it, and updating the templates. So as part of getting the initial spec URLs added (for the docs where we already have them, but in old-style manual tables, and so can automated the process of adding them), I plan to have the draft documentation ready for review. But since the documentation should be informed by real cases, it seems to make sense to first get the spec URLs added for the existing real cases that we have — so then the documentation can reflect whatever info/patterns we can notice from looking at those. |
It looks like this is representing multiple spec URLs as a comma-separated string rather than an array of strings, e.g. https://github.com/mdn/content/pull/13198/files#diff-fc3c88bc0f74503f8b68c76ebff9648624ba3f0985d0d9f59b141f5ff7b4ada2R11-R13. Is there a reason we are doing it this way? It seems harder to read and harder to work with. |
Yeah, I agree it’s better to have a YAML array for the multiple-URLs cases. The YAML multi-line string thing is not ideal. Sorry, when I wrote the code, I just hadn’t thought much about the multiple-URL case. Anyway, at this point I’ll write up a patch for the code to make it output a YAML array for the multiple-URLs cases — as well as writing patches for the existing cases of multiple-URL articles sources that’ve already been merged. |
Sorry to put you to the trouble Mike. But I do think it's worth addressing. |
Definitely — thanks for catching it |
So as far as I can tell, this consuming code in Yari appears to work fine as-is if the Given that, it seems like there’s nothing to change in this existing consumer code that’s already merged into Yari. That said, over in #5309, I did need to change my (unmerged) modifications to the Lines 244 to 246 in 26f5d2c
But, for common case where there’s only one spec URL, that code still outputs a YAML string on a single line, like this: spec-urls: https://w3c.github.io/webcrypto/#dfn-AesCbcParams And since — as mentioned above — the existing consuming code in Yari that’s already been merged works fine both when the Given that, I think we should allow authors to use a string for the
And even if we did require that, I can imagine that even when writing docs ourselves, we might sometimes forget that we’re supposed to use a YAML array there, and we’d just instead use a string. But the thing is, since that would still work, we wouldn’t even know we were violating the use-an-array rule — unless we were somehow linting for it. In fact, given that the existing code also works as expected even in the case that somebody gives the Anyhow, that’s just a long-winded way of saying that I think we don’t need to change anything in the code, and we can just have the documentation say
|
I agree with this. Let's wait for Will's feedback and we can move again forward on the content side. |
Yes, I think we should say
@hamishwillee had some interesting questions.
Sort of, except this is YAML, so: spec_urls:
- https://drafts.csswg.org/css-inline/#alignment-baseline-property
- https://svgwg.org/svg2-draft/text.html#AlignmentBaselineProperty
Ideally we would have the same validation for these spec URLs as we have for BCD ones. But I think that is not in place at the moment.
In the version implemented by this PR, it looks like if both BCD and if the This makes things much clearer for authors.
Great point, I had forgotten about that. IIRC the main function of that argument was a migration aid while we were adding Right now there are only a few instances of it: Instances of "{{Specifications("a key")}}"./files/en-us/web/web_components/index.md:{{Specifications("html.elements.template")}} ./files/en-us/web/web_components/index.md:{{Specifications("api.ShadowRoot")}} ./files/en-us/web/api/css_typed_om_api/index.md:{{Specifications("api.CSSStyleValue")}} ./files/en-us/web/api/console_api/index.md:{{Specifications("api.console")}} ./files/en-us/web/api/css_properties_and_values_api/index.md:{{Specifications("api.CSS.registerProperty")}} ./files/en-us/web/api/web_audio_api/index.md:{{Specifications("api.AudioContext")}} ./files/en-us/web/api/htmlelement/change_event/index.md:{{Specifications("api.GlobalEventHandlers.onchange")}} ./files/en-us/web/api/background_tasks_api/index.md:{{Specifications("api.Window.requestIdleCallback")}} ./files/en-us/web/api/web_share_api/index.md:{{Specifications("api.Navigator.share")}} ./files/en-us/web/api/remote_playback_api/index.md:{{Specifications("api.RemotePlayback")}} ./files/en-us/web/api/canvas_api/index.md:{{Specifications("html.elements.canvas")}} ./files/en-us/web/api/css_painting_api/index.md:{{Specifications("api.PaintWorkletGlobalScope")}} ./files/en-us/web/api/compression_streams_api/index.md:{{Specifications("api.CompressionStream")}} ./files/en-us/web/api/css_counter_styles/index.md:{{Specifications("api.CSSCounterStyleRule")}} ./files/en-us/web/api/geolocation_api/index.md:{{Specifications("api.Geolocation")}} ./files/en-us/web/api/badging_api/index.md:{{Specifications("api.Navigator.setAppBadge")}} ./files/en-us/web/api/notifications_api/using_the_notifications_api/index.md:{{Specifications("api.Notification")}} ./files/en-us/web/api/background_synchronization_api/index.md:{{Specifications("api.SyncManager")}} ./files/en-us/web/api/cookie_store_api/index.md:{{Specifications("api.CookieStore")}} ./files/en-us/web/api/serviceworkerglobalscope/sync_event/index.md:{{Specifications("api.SyncEvent")}} ./files/en-us/web/api/broadcast_channel_api/index.md:{{Specifications("api.BroadcastChannel")}} ./files/en-us/web/api/xmlhttprequest/using_xmlhttprequest/index.md:{{Specifications("api.XMLHttpRequest")}} ./files/en-us/web/api/xmlhttprequest/html_in_xmlhttprequest/index.md:{{Specifications("api.XMLHttpRequest")}} ./files/en-us/web/api/visual_viewport_api/index.md:{{Specifications("api.VisualViewport")}} ./files/en-us/web/api/beacon_api/index.md:{{Specifications("api.Navigator.sendBeacon")}} ./files/en-us/web/api/credential_management_api/index.md:{{Specifications("api.Credential")}} ./files/en-us/web/api/css/factory_functions/index.md:{{Specifications("api.CSS")}} ./files/en-us/web/api/channel_messaging_api/index.md:{{Specifications("api.MessageChannel")}} ./files/en-us/web/api/channel_messaging_api/using_channel_messaging/index.md:{{Specifications("api.MessageChannel")}} ./files/en-us/web/api/resize_observer_api/index.md:{{Specifications("api.ResizeObserver")}} ./files/en-us/web/api/contact_picker_api/index.md:{{Specifications("api.ContactsManager")}} ./files/en-us/web/api/clipboard_api/index.md:{{Specifications("api.Clipboard")}} ./files/en-us/web/api/constraint_validation/index.md:{{Specifications("api.ValidityState")}} ./files/en-us/web/api/content_index_api/index.md:{{Specifications("api.ContentIndex")}} ./files/en-us/web/http/headers/want-digest/index.md:{{Specifications("http.headers.Want-Digest")}} ./files/en-us/web/http/headers/digest/index.md:{{Specifications("http.headers.Digest")}} ./files/en-us/web/css/break-inside/index.md:{{Specifications("css.properties.break-inside.multicol_context")}} ./files/en-us/web/css/column-gap/index.md:{{Specifications("css.properties.column-gap.grid_context")}} ./files/en-us/web/css/break-before/index.md:{{Specifications("css.properties.break-before.multicol_context")}} ./files/en-us/web/css/align-items/index.md:{{Specifications("css.properties.align-items.grid_context")}} ./files/en-us/web/css/gap/index.md:{{Specifications("css.properties.gap.grid_context")}} ./files/en-us/web/css/row-gap/index.md:{{Specifications("css.properties.row-gap.grid_context")}} ./files/en-us/web/css/break-after/index.md:{{Specifications("css.properties.break-after.multicol_context")}} ./files/en-us/web/css/justify-self/index.md:{{Specifications("css.properties.justify-self.grid_context")}} ./files/en-us/web/css/align-self/index.md:{{Specifications("css.properties.align-self.grid_context")}} ./files/en-us/web/css/justify-items/index.md:{{Specifications("css.properties.justify-items.grid_context")}} ./files/en-us/web/css/align-content/index.md:{{Specifications("css.properties.align-content.grid_context")}} ./files/en-us/web/css/justify-content/index.md:{{Specifications("css.properties.justify-content.grid_context")}} ./files/en-us/mdn/structures/specification_tables/index.md:{{Specifications("css.properties.text-align")}} ./files/en-us/mdn/structures/specification_tables/index.md:you can pass the browser-compat query directly to the macro: `\{{Specifications("css.properties.text-align")}}` ./files/en-us/mdn/structures/page_types/api_landing_page_template/index.md:{{Specifications("path.to.feature.Interface_1")}} ./files/en-us/mdn/structures/page_types/api_landing_page_template/index.md:{{Specifications("path.to.feature.Interface_2")}} There seem to be a few different cases here. Most of these seem to be for pages that don't have a BCD entry, like the API overview pages. We should be able to replace these usages with Some more are for the CSS properties like But there are a few usages that are tricky. For instance: https://developer.mozilla.org/en-US/docs/Web/Web_Components#specifications . Here it seems like we want to say:
I think we need to decide if we want to support this kind of thing or not. I would personally try to avoid it (Web Components is a weird page that is pretending to be an API overview page but isn't, really). But if we do decide to allow this kind of arbitrary inclusion of spec tables, then it should trump anything else, and say: if |
@wbamberg There is a lot of good points in ^^^ but I worry that @schalkneethling will miss them as this is a closed PR. I like the idea of getting rid of I very much like both spec-url and bcd key to be reported as a flaw. |
Yeah, I have been thinking the same thing. So I think we should update our BCD processing code to handle |
About making it an error for a document to have The general case is when there’s a So, allowing |
My main concern here is simplicity for authors. This is looking like a pretty complicated system right now with lots of different ways to provide a spec URL. It's fine to have some extra complexity as a transitional thing, if we have a desired final state that we like, and a plan for getting there. So I think we need (1) a description of where we want to end up with spec URLs, (2) a description of where we are now, and (3) a plan for how to get from here to there. I'm happy to try to start this off if you like. |
Hey @wbamberg and @sideshowbarker. I have been following along here as much as I can, feel free to move this to either a discussion or new issue and link it here. Thanks! |
Yes, please, if you could start it off, that’d be great, and much appreciated. |
I'm stealing this a little. I went ahead and wrote up a possible vision for this stuff: #5347. That might cover (1), but not (2) or (3), but I think (3) is the last piece anyway. |
Fixes #4704 • Related: #5277
This change adds support for listing specs in the Specifications section based on the value of the
spec-urls
frontmatter key — instead of or in addition to listing specs based on the value of thebrowser-compat
frontmatter key, or the value of the parameter passed to theSpecifications
macro.