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

Update instructions for adding new macros #10513

Merged
merged 7 commits into from Dec 23, 2021
Merged

Update instructions for adding new macros #10513

merged 7 commits into from Dec 23, 2021

Conversation

astearns
Copy link
Contributor

Summary

I was looking at how to fix an incorrect spec link, and found some instructions made out-of-date by mdn/yari#4081.

I have updated this section based on what I think is now the correct workflow with an updated example (based on the most-recent PR I found).

Motivation

My main motivation is to verify that what I’ve written is the correct workflow, and secondarily to fix the broken link and stale instructions I encountered.

Supporting details

mdn/yari#4081

Related issues

Metadata

  • Adds a new document
  • [] Rewrites (or significantly expands) a document
  • Fixes a typo, bug, or other error

I was looking at how to fix an incorrect spec link, and found some instructions made out-of-date by mdn/yari#4081. 

I have updated this section based on what I think is now the correct workflow with an updated example (based on the most-recent PR I found).
@astearns astearns requested a review from a team as a code owner November 14, 2021 16:50
@astearns astearns requested review from a team, ddbeck and fiji-flo and removed request for a team November 14, 2021 16:50
@@ -713,21 +713,17 @@ KumaScript macros are still used on MDN pages, even in the new platform.
These are the function names surrounded by handlebars-style double curly
braces that you'll see in the source code on occasion, for example
`{{domxref}}` Eventually we have to replace them with something else,
but they are here for now. They live in <https://github.com/mdn/yari/tree/main/kumascript/macros>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is incorrect. The jsondata did indeed move, but the macros did not, and this sentence is referring to the macros.

I like where you're heading with this though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. Does the latest change make more sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@astearns Yes it makes more sense and this is an improvement.

But even so, I have modified this even more ruthlessly. The only macro for which this is particularly relevant is the API sidebar one (i'm not sure which of the other data is actually used since specdata now comes from browser compatibility table).

So I have updated this to point to highlight that, and point to more detailed instructions on how it that is updated. I removed the particular example PR because to me that is just an example of change, not instruction on how to change.

This section probably needs a link for what each type of data is used for an what macros it effects, but IMO this is good for now.

Thanks very much for highlighting this.

@teoli2003 Deferring to you to further review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW, my original problem was needing to figure out how to use the macro to link to a new spec section. I likely would not have even noticed this part of the ReadMe without the SpecRef.json example.

The spec link and compatibility table are incorrect on this page: https://developer.mozilla.org/en-US/docs/Web/API/CSS

I had been assuming I’d need to create a new SpecRef.json entry for that page, but now I am not sure if that is correct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@astearns Those tables are filled from data in browser compatibility which is specified here in the metadata : https://github.com/mdn/content/blob/main/files/en-us/web/api/css/index.md?plain=1#L10 - see browser-compat: api.CSS

That data is actually defined here: https://github.com/mdn/browser-compat-data/blob/main/api/CSS.json

I am not sure how SpecData is used, which is why I've asked for review of my comment :-).

Copy link
Collaborator

Choose a reason for hiding this comment

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

PS But it does highlight the need to make it clear where browser compat data comes from, if it is not documented in this page. Happy to explore that when we know what the other json files are used for in the tree.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wbamberg I was wondering if you could review this? It's been open for a bit.

I think it is correct. My concern is whether more needs to be done, and that depends a bit on how the json in https://github.com/mdn/content/blob/main/files/jsondata/ is used. I THINK perhaps specdata is no longer used, and perhaps some of the others.
If you don't know, who do you think might?

Copy link
Collaborator

@wbamberg wbamberg Nov 26, 2021

Choose a reason for hiding this comment

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

Sorry, this is going to be a long answer to an apparently simple question.

On the specifics of SpecData: yes, it is still used 😭. It's used by the Spec2 and SpecName macros, which were the old way to build spec tables, before we started keeping spec URLs in BCD. Looking through our content with something like rg Spec2 ./files you'll still see a lot of uses of these macros.

Without more detailed work I don't know how many of these can be removed, and why they can't reference a spec URL in BCD. I do know that some pages can't reference spec URLs in BCD because BCD doesn't have an entry that maps to them - API overview pages, for example. This is related to https://github.com/mdn/content/discussions/4920 and I think my preferred approach there would be to allow spec URLs in the page front matter as a fallback for API pages (and maybe also for other pages whose features aren't in BCD). But like I say, more work is needed there. It would be nice to resolve this.

Most of the time though, spec URL updates should happen in BCD and pages should be able to pull them from there.

On this PR: I do agree that the example given here:

Take #187. Florian wanted to add documentation for a new WebGL extension to MDN...

...is out of date and misleading and needs to be fixed. But this PR changes the section from "Making a change that depends on a macro update" to "Making a change that depends on a JSON data update", and these are not the same thing at all. Many macro updates are not updates to files under "files/jsondata". For example, adding a new page in the Learning Area requires a corresponding update to the sidebar, which is a macro under yari/kumascript.

Also though, this README section is short and vague enough as to be not much help for contributors who don't already understand the byzantine complexity of MDN's authoring system. So they're going to have to ask for help anyway.

So I think there are basically two options for this issue:

  • just replace that example with one about updating a sidebar. Then at least it's not actively misleading.

  • update that section to be something like "Making a change that depends on external content", and say something like: sometimes MDN content updates require updates to other files and repositories, such as KS macros, or JSON data, or mdn/data, or external example repos like mdn/learning-area. In these situations make the PR to the other repo first and get that merged. I guess an exception actually is JSON data now, because it's now in the content repo too so can be updated on the same PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @wbamberg - sounds like good advice. I've gone with option 2 - is this now better?

@hamishwillee hamishwillee dismissed their stale review November 15, 2021 00:10

Because it has been addressed.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

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

👍 thanks for the updates, @astearns and @hamishwillee !

@wbamberg wbamberg merged commit 794d084 into mdn:main Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants