-
Notifications
You must be signed in to change notification settings - Fork 175
Use data/master for CSS_Ref and CSSRef #132
Conversation
I currently don't have the possibility to test this, but I assume the syntaxes now need to be HTML encoded before adding them to the output. Sebastian |
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.
Tested locally after mdn/data#52 on three pages:
- Web/CSS/border-bottom-right-radius - same output
- Web/CSS/@font-face - same output
- Web/CSS/@counter-style/additive-symbols - Uses
&&
, inlines<rgb-component>
,<named-color>
. - Web/CSS/:nth-child - different output, expected.
The previous output was:
:nth-child( <an-plus-b> [ of <selector># ]? ) { style properties }
The new output drops { style properties }
, and changes the handling of An+B:
:nth-child( <nth> [ of <selector># ]? )
I think this works, and we can adjust in a future PR.
The <pre> output worked better than expected, so here's the current syntax box on @counter-style/additive-symbols: [ <integer> && <symbol> ]# and here's the new one: [ <integer> && <symbol> ]# |
Thanks for your review. To make this PR more complete, I would say we'll need a new commit here to use "master" of mdn/data again. That way there is also a single PR that needs reverting, in case things go wrong. |
This PR does use the "master" branch: You may need to rebase on master to pick up the current "mdn" branch setting, so your change back to "master" will be part of the PR. |
Rebased and added a commit that changes back to "master" of mdn/data. |
I'd be more comfortable restricting the mdn/data/master change to |
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.
After looking at the output, I think two of the macros are ready to go:
- CSSAnimatedProperties
- csssyntax
The others will require some adjustments, and maybe fix some missing data, before they can be switched from the mdn/data/mdn
branch to the mdn/data/master
branch.
My suggestion is to keep these macros on mdn/data/mdn
, removing them from the PR, which can then be merged. Then create new PRs for each of the remaining macros.
macros/CSSAnimatedProperties.ejs
Outdated
var selectorsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/selectors.json"; | ||
var typesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/types.json"; | ||
var syntaxesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/syntaxes.json"; | ||
var unitsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/units.json"; |
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.
👍 tested on Web/CSS/CSS_animated_properties, same output
var selectorsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/selectors.json"; | ||
var typesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/types.json"; | ||
var syntaxesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/syntaxes.json"; | ||
var unitsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/units.json"; |
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.
The sidebar renders, but just has the sections (CSS, CSS Reference, CSS Box Model) on Web/CSS/box-shadow. Further testing is needed with a bigger database (staging, anon DB dump)
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.
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.
There is code like var pageList = page.subpagesExpand('/en-US/docs/Web/CSS');
. A method that calls the $children?expand
endpoint of Kuma. The pageList
variable is empty for me, so something is wrong locally with this. I do have a few subpages and http://localhost:8000/en-US/docs/Web/CSS$children?expand works as expected. Maybe caching in KS, I don't know how to fix that. It might just work fine on staging.
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.
There's a bug in pages.subpagesExpand()
and similar page macros. Will opened bug 1368134, and that should cover the work. If I alter DekiScript-Page.ejs
to use api:8000
vs p.host
, the child call works and the sidebar is populated.
var selectorsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/selectors.json"; | ||
var typesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/types.json"; | ||
var syntaxesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/syntaxes.json"; | ||
var unitsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/units.json"; |
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.
Tested on Web/CSS/Reference. Renders, with changes:
Removed links:
- annotation()
cubic-bezier()
format()
andformat() (@font-face)
local()
ornaments()
rect()
repeat()
steps()
styleset()
@stylistic
stylistic()
swash()
:unresolved
Added Links:
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 changed the regex in getItemsFromSyntax
. Not sure this is a good way to get these items, but I think it fixes the current code.
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've confirmed that the removed links are back. There are now two added links in dev:
macros/cssinfo.ejs
Outdated
var typesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/types.json"; | ||
var syntaxesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/syntaxes.json"; | ||
var unitsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/units.json"; | ||
var localStringsUrl = "https://raw.githubusercontent.com/mdn/data/master/l10n/css.json"; |
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.
Tested on Web/CSS/outline-offset. Renders the same, except that the initial value is no longer wrapped in a <code>
element.
Merged the changes for CSSAnimatedProperties and csssyntax as #155. |
If I'm correct, the last two macros to change back to data/master are CSS_Ref.ejs and CSSRef.ejs. This could be done in this PR. |
@jwhitlock I would appreciate another look to get us all back to master. |
@Elchi3 sorry I haven't tested this yet. I will take a look Tuesday. |
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.
👍 It looks like these macros work, except for the page bug, which we should cover in a different PR.
var selectorsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/selectors.json"; | ||
var typesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/types.json"; | ||
var syntaxesUrl = "https://raw.githubusercontent.com/mdn/data/master/css/syntaxes.json"; | ||
var unitsUrl = "https://raw.githubusercontent.com/mdn/data/master/css/units.json"; |
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.
There's a bug in pages.subpagesExpand()
and similar page macros. Will opened bug 1368134, and that should cover the work. If I alter DekiScript-Page.ejs
to use api:8000
vs p.host
, the child call works and the sidebar is populated.
- mdn/kumascript#132 - CSS_Ref, CSSRef: mdn/data master branch - mdn/kumascript#186 - GroupData: Update WebVR sidebar - mdn/kumascript#187 - LegacyAddonsNotice: Improve notice
This PR intends to prepare MDN for the changes made in mdn/data#52.
There might be more that is necessary. I'm still working through the changes in mdn/data#52.