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

Support multiple BCD queries #6004

Closed
wants to merge 6 commits into from

Conversation

caugner
Copy link
Contributor

@caugner caugner commented Apr 19, 2022

Summary

Alternative implementation of #5972 to allow frontmatter to reference multiple BCD queries.

Problem

Previously, pages could only reference a single BCD query.

Solution

  1. Refactored document-extractor.js to use pure functions for BCD and Specifications.
  2. Extracted BCD_BROWSERS and BCD_BROWSER_RELEASES constants.
  3. Combine multiple BCD queries into a common hierarchy (96aa5e2).

Screenshots

Example combining html.elements.article and html.elements.dialog:

image


How did you test this change?

  1. Updated mdn/content/files/en-US/web/html/element/article/index.md as follows:
-browser-compat: html.elements.article
+browser-compat:
+  - html.elements.article
+  - html.elements.dialog

@caugner caugner added the browser-compat issues related to the browser compatibility data tables (BCD) label Apr 19, 2022
@caugner
Copy link
Contributor Author

caugner commented Apr 19, 2022

@sideshowbarker Please have a look at this alternative implementation of your PR.

However, the result is not the same, as your PR results in separate sections:

image

@sideshowbarker
Copy link
Member

@sideshowbarker Please have a look at this alternative implementation of your PR.

However, the result is not the same, as your PR results in separate sections

I think the output your approach produces is better. I realize now that for the case where a BCD table contains data for both a feature and its subfeatures, we ideally should have already been presenting the subfeatures in the table in such a way that it’s visually clear they’re subfeatures. So I like how that’s visually indicated in the output here — although I think as currently designed it may still be a bit too subtle for readers to notice. Maybe there’s some way to make it even a bit more clear — but I don’t have a lot of skill with page design, so I can’t offer any specific suggestions, and I’d still be quite happy with it as-is.

As far as I can see, the only thing we lose with this approach as currently written is that even when we have multiple features, there’s only one “Report problems with this compatibility data on GitHub” link — and the metadata that gets added to the resulting issue has the MDN URL for the first feature only. I guess it could be refined to include the MDN URLs for all the features, but I’m not sure if it merits the extra effort — I think it’s a minor loss, and worth the tradeoff.

Anyway, I think the refactoring here and resulting code are outstanding — so thanks extremely much for writing this up. I’d be super happy to see it land.

@github-actions
Copy link
Contributor

This pull request has merge conflicts that must be resolved before we can merge this.

@github-actions github-actions bot added the merge conflicts 🚧 Please rebase onto or merge the latest main. label May 16, 2022
@sideshowbarker
Copy link
Member

@caugner Is there anything blocking this?

…I mean other than the merge conflict, which we I believe can resolve (see below).

If nothing other than the merge conflict is blocking it, can we move forward with merging it? (After resolving the conflict)

As far as the merge conflict: I rebased this branch against main, applied my suggestion in #6004 (comment) and pushed to the https://github.com/mdn/yari/tree/sideshowbarker/support-multiple-bcd-queries-caugner-rebased branch — and I tested that branch and found it to be working as expected for all cases.

(I didn’t push it here yet because I figured you might want to first have a separate view without the existing commits here being written over by the rebase — but if you prefer I do actually push it here for testing, I’d be happy to do that.)

Comment on lines +553 to +565
if (data) {
// If 'data' is non-null, that means we have data for a BCD feature
// that we can extract spec URLs from.
for (const [key, compat] of Object.entries(data)) {
if (key === "__compat" && compat.spec_url) {
if (Array.isArray(compat.spec_url)) {
specURLs = compat.spec_url;
} else {
specURLs.push(compat.spec_url);
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (data) {
// If 'data' is non-null, that means we have data for a BCD feature
// that we can extract spec URLs from.
for (const [key, compat] of Object.entries(data)) {
if (key === "__compat" && compat.spec_url) {
if (Array.isArray(compat.spec_url)) {
specURLs = compat.spec_url;
} else {
specURLs.push(compat.spec_url);
}
}
}
}
if (data) {
// If 'data' is non-null, that means we have data for one or more BCD
// features that we can extract spec URLs from.
//
// If we’re processing data for just one feature, then the 'data'
// variable will have a __compat key. So we get the one spec_url value
// from that, and move on.
//
// (The value may contain data for subfeatures too — each subfeature
// with its own __compat key that may contain a spec_url — but in that
// case, for the purposes of the Specifications section, we don’t want
// to recurse through all the subfeatures to get those spec_url values;
// instead we only want the spec_url from the top-level __compat key.
if (data.__compat) {
const compat = data.__compat;
if (compat.spec_url) {
if (Array.isArray(compat.spec_url)) {
specURLs = compat.spec_url;
} else {
specURLs.push(compat.spec_url);
}
}
} else {
for (const block of Object.values(data)) {
// If we reach here, we’re processing data for two or more features
// and the 'data' variable will contain multiple blocks (objects) —
// one for each feature — each with its own __compat key
const compat = block.__compat;
if (compat.spec_url) {
if (Array.isArray(compat.spec_url)) {
specURLs = compat.spec_url;
} else {
specURLs.push(compat.spec_url);
}
}
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Without this change, the code doesn’t correctly output the Specifications section when the browser-compat value contains multiple BCD queries.

@sideshowbarker
Copy link
Member

I’ve now made so many further changes on top of this PR that it seems like it makes sense to raise a new PR to supersede it.

So I’ve done that now in #6358.

@caugner
Copy link
Contributor Author

caugner commented Jun 3, 2022

Closing in favor of #6464.

@caugner caugner closed this Jun 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser-compat issues related to the browser compatibility data tables (BCD) merge conflicts 🚧 Please rebase onto or merge the latest main.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants