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

feat(css-syntax): render more constituents #11003

Merged
merged 7 commits into from
May 2, 2024
Merged

Conversation

fiji-flo
Copy link
Contributor

Summary

The current code only considers css types as constituents. However, e.g. margin has a property as constituent.

This change allows us to render constituents of type property.


Screenshots

Before

image

After

image


How did you test this change?

Locally

Changed:
http://localhost:3000/en-US/docs/Web/CSS/margin#formal_syntax
http://localhost:3000/en-US/docs/Web/CSS/padding#formal_syntax
Did not change:
http://localhost:3000/en-US/docs/Web/CSS/gradient#formal_syntax

The current code only considers css types as constituents.
However, e.g. `margin` has a property as constituent.

This change allows us to render constituents of type property.
@fiji-flo fiji-flo requested a review from a team as a code owner April 25, 2024 12:05
@github-actions github-actions bot added the macros tracking issues related to kumascript macros label Apr 25, 2024
@fiji-flo
Copy link
Contributor Author

@wbamberg I'd value your input a lot since you're the expert here 🙏

@caugner
Copy link
Contributor

caugner commented Apr 25, 2024

@fiji-flo You might want to run yarn test:kumascript -u to update the jest snapshots.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

LGTM, but didn't test locally.

@fiji-flo
Copy link
Contributor Author

@fiji-flo You might want to run yarn test:kumascript -u to update the jest snapshots.
They actually found a but which I first fixed 😮‍💨

@caugner
Copy link
Contributor

caugner commented Apr 25, 2024

Btw I guess this is a fix?

@wbamberg
Copy link
Collaborator

wbamberg commented Apr 25, 2024

It's good to handle constituent properties in formal syntax. But rather than expanding out the syntax for properties I think it would be better to link to the property's own page, which lists its own formal syntax. This is in line with what we do for data types like <length>, where we have a separate page that lists the syntax for that type (e.g. https://developer.mozilla.org/en-US/docs/Web/CSS/margin-left#formal_syntax).

Expanding out property syntax like this might make lots of syntaxes very long and repetitive. Also I think linking helps to emphasise to readers that this is the same as the property syntax (that essentially the shorthand is defined in terms of its longhands).

@fiji-flo
Copy link
Contributor Author

@wbamberg I thought about just linking and that should not be my decision to make. Just as a explanation why I went with expanding: I saw this especially with the shorthand properties like inset, padding ... and I thought it would be beneficial to inline it. I can generate a diff of the syntax sections and share it, but I'm also fine with changing it to links.

@caugner
Copy link
Contributor

caugner commented Apr 26, 2024

@fiji-flo @wbamberg Maybe showing those as a <details> element would be an option to save vertical space?

@caugner caugner changed the title feat(css-sytax): render more constituents feat(css-syntax): render more constituents Apr 26, 2024
@wbamberg
Copy link
Collaborator

wbamberg commented Apr 26, 2024

@wbamberg I thought about just linking and that should not be my decision to make. Just as a explanation why I went with expanding: I saw this especially with the shorthand properties like inset, padding ... and I thought it would be beneficial to inline it. I can generate a diff of the syntax sections and share it, but I'm also fine with changing it to links.

If I were trying to make the call I'd love to know lots of places where this would make a difference, and how big a difference it would make, and especially whether there would be many places where we'd get pathologically huge formal syntax. I could imagine font and grid for instance might get pretty huge, and maybe mask as well. But maybe it's rare enough to be acceptable?

It's one of these tricky things where the optimal choice for 95% of cases ends up being pathological for the remaining 5%, and the optimal choice for the 5% is worse for the 95%.

Maybe it would be worth thinking about suppressing inline expansion in a few ad hoc cases, as we already do for <gradient> and <color>.

@fiji-flo @wbamberg Maybe showing those as a <details> element would be an option to save vertical space?

The pretty-printing and syntax highlighting added in #4656 was really just an incremental improvement over what was there before, and I think there's definitely scope to present this in a more interactive way. ISTR css-tree used to have a thing where you could explore syntax by clicking on the components and it would expand to show you the details of that component. But getting it all right, and usable by people, feels like a project.

I think I experimented with using some <details> when I did that but felt that in most cases showing everything would be more usable.

@fiji-flo
Copy link
Contributor Author

@wbamberg finally the diff https://static.fiji-flo.de/

@wbamberg
Copy link
Collaborator

Thanks, that's a very usable diff. I think it looks great. font and grid for instance are longer but still OK to my eye, and they're much more informative.

I'm totally +1 on this.

@estelle
Copy link
Member

estelle commented May 1, 2024

@wbamberg I thought about just linking and that should not be my decision to make. Just as a explanation why I went with expanding: I saw this especially with the shorthand properties like inset, padding ... and I thought it would be beneficial to inline it. I can generate a diff of the syntax sections and share it, but I'm also fine with changing it to links.

I think expanding is the right way to go. Thanks. 👍

That said, if a value reads <'prop-name'>, it would be great for prop-name to be a link to the property.'s page. Not a blocker, but if we could add that as an additional feature (keeping the expanded values for that property as part of the shorthand's formal syntax, that would be 👍 x 💯

@wbamberg
Copy link
Collaborator

wbamberg commented May 1, 2024

That said, if a value reads <'prop-name'>, it would be great for prop-name to be a link to the property.'s page. Not a blocker, but if we could add that as an additional feature (keeping the expanded values for that property as part of the shorthand's formal syntax, that would be 👍 x 💯

Yes, I had that thought too but forgot to mention it, thanks Estelle. Linking as well as expanding would be even better.

@fiji-flo
Copy link
Contributor Author

fiji-flo commented May 1, 2024

I linked them, but the whole <'prop-name'> to be more consistent with the other links. If we want to change that I'd do it across the board but in a follow up.
Here's the updated diff: https://static.fiji-flo.de/linked.html

@fiji-flo
Copy link
Contributor Author

fiji-flo commented May 2, 2024

@wbamberg @estelle are you okay with the latest change (how the links look). I also found other issues like the duplicate on https://developer.mozilla.org/en-US/docs/Web/CSS/calc-sum#formal_syntax which I will address in a follow up.

@estelle
Copy link
Member

estelle commented May 2, 2024

@wbamberg @estelle are you okay with the latest change (how the links look). I also found other issues like the duplicate on https://developer.mozilla.org/en-US/docs/Web/CSS/calc-sum#formal_syntax which I will address in a follow up.

Yes. LGTM. Thank you!

@wbamberg
Copy link
Collaborator

wbamberg commented May 2, 2024

@wbamberg @estelle are you okay with the latest change (how the links look). I also found other issues like the duplicate on https://developer.mozilla.org/en-US/docs/Web/CSS/calc-sum#formal_syntax which I will address in a follow up.

Yes, looks good to me, too. Thanks!

@caugner caugner merged commit 9c61077 into main May 2, 2024
14 checks passed
@caugner caugner deleted the enhance-css-formal-syntax branch May 2, 2024 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macros tracking issues related to kumascript macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants