feat(macros): rewrite CSSSyntax macro #4656
Conversation
|
This is definitely the way this should go in my opinion. Great improvement over the current regex parsing. and I really like the pretty printing, makes everything much more readable. Just a few notes:
webref seems to have syntax for at-rules and selectors. for example @media. The only thing is that the top
These links helped me quite a lot over the years, and I think it would be a shame to lose them.
I think that like you said, internal links are less important, but the external ones are very helpful, current version seems to build links to
In my opinion links are more important than syntax coloring, maybe there is some combination that would keep the link accessible while still styling it in a way to emphasis the syntax. |
|
Thanks for your kind and helpful comments!
I will look into this some more.
You're right of course, I'm just being lazy.
I think it's best to keep it simple and always link, then if the target is missing file a bug to write the page! Anyway we could see how many broken links we will get, and how much work it would be to write them all.
I just pushed an update that adds links, and they of course get underlined, so maybe it's not so bad. |
Just pushed an update that adds this back. |
It looks like we can map at-rule descriptors, like, say, Similarly for selectors: although there is underlying syntax for things like @dontcallmedom , do you know how we can do this using webref? |
|
Another issue with this change is that you are flattening the properties and the value-spaces into a single record, overriding duplicates between specs. The current version in mdn takes the a few examples of differences: <'border-bottom-color'> is currently and in webref:
<'align-self'> is currently and in webref:
<'text-align'> is currently and in webref:
<'margin-top'> is currently and in webref:
There are a few issues:
List of duplicate/updated syntax between specs (ordered in according to `webref/css` listAll )
|
schalkneethling
added
🧑🤝🧑 community
|
Some thoughts:
Could we litmus test it - remove formal syntax macro from pages this doesn't support and see if we get any complaints. This isn't a particular kind way of trying it, but we don't want to be stuck in a world of using both webref & data
Yeh as far as I can tell this isn't kept up to date in data anyway so should be ok
Probably yes - having trawled through CSS pages more over the past few weeks we could generally do a review & tidy anyway.
Maybe @schalkneethling could take a look +1 pretty printing, it looks much better with the auto new lines & better colors
I'd be interested to see how many pages this affects, eg how many are very long and how many are actually not that long For all the other comments maybe we can push for changes to webref based on our needs? In short this still looks like a much better solution than we have, even if a couple of things temporarily regress. I don't see anything that's a blocker 👍 |
|
I did a teeny bit of asking CSS devs I knew how they felt about the 'Formal Syntax' sections and most either found them confusing or hadn't taken much notice of them - the examples at the top of CSS pages were the bit they found the most useful... The suggestion about details summary to expand them might be a good one 👍 |
This is changing with the redesign so I reckon there is not much sense in me looking at it until that has landed. |
|
Coming back to this...
You're absolutely right, the macro should be cleverer here (although I think "curated" is perhaps a generous way to describe the choices made in mdn/data). Perhaps we could do something like: we already link to a specification in the page. We could say that this represents our configured choice, and the macro could display formal syntax from that spec. So in the case above, I like this because there are no additional config choices to make - the config choice is the spec we list. We do sometimes list more than one spec (e.g. |
|
I just pushed some updates to this PR. In #4656 (comment), @idoros raised a good question: in webref, we sometimes have multiple different definitions of the same property, from different specifications. What should we do in this case? My suggestion was that we could use spec URLs in BCD to decide which webref spec to use. Because it seems like a good idea for the formal syntax we display to match the spec we link to. One issue here would be: what if we have multiple spec URLs in BCD? I haven't looked into this much yet, because I'd like first to figure out how often this happens and why. But another issue is that sometimes the spec URL in BCD does not exactly match a spec URL in webref. For example, in But, for example, What should we do in this case? The macro could map the BCD spec URL onto the highest-level corresponding spec in webref, maybe? So @dontcallmedom , @Elchi3 , @Rumyra etc, I'd love to hear what you all think about this. |
|
Another thing is that some entries in webref don't have a In this case there are two other specs in webref that could provide a "base value" for I'm not sure how in general we should figure out which of these base values to choose. There aren't all that many of these Does this make sense, or am I confused? |
|
copying @tidoust |
Mapping the unnumbered version of a draft to its numbered one can be obtained via the info in browser-specs - e.g. if you look up the That said, there may be a worthy discussion to be had about MDN/BCD using unnumbered versions - I'm not sure there is a good mechanism for MDN to track that this unnumbered version is now pointing to something else, and that the page/the BCD data may no longer reflect the content of the new version (although I guess it's more general problem of determining of MDN tracks the content of specs; maybe the formal syntax is actually a way to determine whether it does or not: in particular, maybe BCD could detect when it doesn't have data about new syntactic token identified from the formal syntax?).
That's correct. @tidoust, I wonder if this points to having a consolidated view of CSS (the same way we have one for WebIDL)? |
github-actions
bot
added
merge conflicts 🚧
|
This pull request has merge conflicts that must be resolved before it can be merged. |
kumascript/macros/CSSSyntax.ejs
Outdated
| const syntaxDescriptions = { | ||
| '*': { | ||
| fragment: 'asterisk', | ||
| tooltip: 'Asterisk: the entity may occur zero, one or several times' |
There was a problem hiding this comment.
should we prepare this for l10n like:
tooltip: mdn.localString({ 'en-US': 'Asterisk: the entity may occur zero, one or several times' }),
github-actions
bot
removed
the
merge conflicts 🚧
|
Thanks @fiji-flo ! I added the mdn.localString calls, and tested it on lots of pages. I also fixed the merge conflicts, but please double check https://github.com/mdn/yari/pull/4656/files#diff-082377deab2cc777cf2c33c661c69bd5e108564a4acb9a377ce02b742562a2b9, since this was an original change I didn't make - Claas made it in 20281d2 and it conflicted with #6487. |
kumascript/macros/CSSSyntax.ejs
Outdated
|
|
||
| const webRefData = require('@webref/css'); | ||
| const { definitionSyntax } = require('css-tree'); | ||
| const replaceAll = require('string.prototype.replaceall'); |
There was a problem hiding this comment.
Sorry I missed this. We don't need this as we're now on node >= 16
|
-> 69c852a |
|
I just realised that there is a bug here: when we are extracting syntax from webref, although we calculate I just pushed 42fca89 which ought to take care of that. |
| } | ||
| const newValues = parsedWebRef[specName].properties[propertyName].newValues; | ||
| if (newValues) { | ||
| newSyntaxes += ` | ${newValues}`; |
|
Wohoo!!! Awesome to see this go through :D |
@wbamberg FWIW, webref is landing support for at-rules now, e.g https://github.com/w3c/webref/blob/07b36e04df4f20e65aaed3a33f091fe8428cdf4a/ed/css/css-fonts-5.json#L23 |
Thank you @dontcallmedom ! MDN is now fetching and rendering formal syntax for at-rules and at-rule descriptors -> mdn/content#22167 ! |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This is a rewrite of the CSSSyntax macro, with three main changes:
1 use webref instead of mdn/data
2 use css-tree instead of a pile of regexes
3 implement basic pretty-printing and syntax highlighting
The motivation here was mostly to use webref, but it's a lot easier and safer to rewrite the macro to use css-tree than to try to modify the existing one. The extra features we could perhaps have deferred to another PR, but I thought it would be nice for this change to include something user-visible.
Things this version doesn't do that the old one did
this version only works with CSS properties, not selectors or at-rules. This is because webref doesn't include data for anything except properties (as far as I could see, but maybe I am wrong?) I think it's fine to limit formal syntax to properties, but we'd need to remove macro calls from unsupported pages.
this version doesn't have any support for l10n (except for making sure links will work). I was going to add something but it turns out that the current macro only supports "de", so dropping support doesn't seem like a regression.
this version doesn't include tooltips and links for combinators (e.g. "||, "|" etc), only for multipliers ("*", "?" etc). That's because css-tree doesn't give me easy access to those things. We could do some regex hackery to fix that, if it seems important.
this version currently doesn't include links from types, either internal links to further-down-the-formal-syntax or to dedicated pages like https://developer.mozilla.org/en-US/docs/Web/CSS/angle. This is mostly because I'd like to understand what we would like to do here. I think the internal links are not very useful but the links to external pages are - but of course we don't have pages for all types, so this implies that the macro would need a list of all the pages we have, which seems hacky and hard to maintain [Update: thinking about this some more: we should probably have pages for all these types, so maybe we should just create links to all of them and write pages for those that don't exist]. Also link styling is at odds with syntax highlighting, and I'm not sure how best to resolve that.
Things this version does do that the old one didn't
pretty-printing: it does really simple pretty-printing: basically if any type's combinator is anything other than " " (juxtaposition) then it renders the type in separate lines, aligning them, at the top-level only. So rather than:
you get:
There's definitely room to think about what would be ideal here, but I think this greatly reduces the wall of text feel of the formal syntax.
syntax highlighting: again, really basic, just highlighting properties, types, keywords, and functions. I’ve used the same classes that we already use for CSS syntax highlighting, so we get the same colours.
Some before and after
animationgrid-templateOne obvious regression is that the formal syntax is much longer now. If we wanted to we could easily show only the top-level type and hide the rest in a
<details>element. Or we could decide that pretty-printing isn't worth the extra space (but I think it is).