-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
6f6ecaa
Update instructions for adding new macros
astearns 00f18e3
lint fixes
astearns e5b6402
macro/value distinction
astearns c419c7c
values, not strings
astearns 817bfae
Perhaps do it this way?
hamishwillee 4aac803
Update to: Making a change that depends on external content
hamishwillee 353b80a
Fix md-lint errors, hopefully
wbamberg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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.
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.
I see what you mean. Does the latest change make more sense?
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.
@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
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.
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.
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.
@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 :-).
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.
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.
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.
@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?
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.
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 theSpec2
andSpecName
macros, which were the old way to build spec tables, before we started keeping spec URLs in BCD. Looking through our content with something likerg 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:
...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.
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 @wbamberg - sounds like good advice. I've gone with option 2 - is this now better?