feat(macros/Compat): Import browser‑compat‑toolkit changes
#1010
Conversation
ExE-Boss
changed the title
browser‑compat‑toolkit changes
|
review?(@jwhitlock, @wbamberg): Can we please get this merged? |
ExE-Boss
force-pushed
the
macros/compat/import-bc-toolkit-changes
branch
from
23eed11 to
a5eeda3
Compare
ExE-Boss
force-pushed
the
macros/compat/import-bc-toolkit-changes
branch
from
fa807a0 to
df172c2
Compare
There was a problem hiding this comment.
I glanced over this PR and tested it locally. It does render and fixes a variety of things. For reviewing, this PR is uncomfortable as there are like 6 different things fixed at once here. If we could split this up, it would be much easier to review and to blame if things regress later on.
My summary of the changes here are:
- typo fixes: "displayBrowers", "unnested", "supprt"
- Make mdn_urls work with anchors
- Have mdn_urls localized properly
- Rewrite entire writeFlagNote function to use info from BCD and localized strings
- Fix a stray </tr>
- Formatting fixes in the entire traverseFeatures function (it looks like no code changes to me, but the diff is hard to read).
Is this correct? Some of this is actually reviewable quite easily (1, 2, 3, 5). Other stuff might need a deeper look (4 and 6). I guess most things come with tests (except typo fixes and stuff that doesn't need tests).
So, I believe, if you would split out and explain these changes in separate PRs, we can ship many of these things and review other things more in depth. What do you think about that? Thanks for your work.
| @@ -37,6 +37,7 @@ var output = ''; | |||
| var category = query.split(".")[0]; | |||
| var legendItems = new Set(); // entries will be unique | |||
|
|
|||
| // TODO: Migrate localization to Fluent ( https://www.npmjs.com/package/fluent ) | |||
There was a problem hiding this comment.
Please remove this TODO. I don't think we decided to use Fluent. A decision likely requires an ADR. https://github.com/mdn/mdn/tree/master/ADRs
|
Number 6 is a result of whitespace changes, which I’ve moved to #1084. |
|
I’ve now split the changes into the following PRs: |
|
Thank you! |
Split into:
mdn_urlhandling: fix(macros/Compat): Improvemdn_urlhandling #1085</tr>tag: fix(macros/Compat): Fix stray closing</tr>tag #1086writeFlagsNote(…)function: refactor(macros/Compat): RewritewriteFlagsNote(…)function #1087This PR imports the following browser-compat-toolkit changes that I made:
</tr>tag browser-compat-toolkit#7 (moved to fix(macros/Compat): Fix stray closing</tr>tag #1086)mdn_urlhandling #1085)writeFlagsNote(…)function #1087)Features blocked by this:
pref_url(feat(browsers): Addpref_urlfor feature flags page browser-compat-data#3407; moved to refactor(macros/Compat): RewritewriteFlagsNote(…)function #1087)<=syntax (Remove "yes" as an option browser-compat-data#3021)See also:
mdn‑browser‑compat‑toolkitto render the compatibility tables on MDN browser-compat-data#2354review?(@a2sheppy, @chrisdavidmills, @davidflanagan, @Elchi3, @escattone, @jwhitlock, @MatonAnthony, @wbamberg)