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

Add support for rest of the CSS features and upgrade to 6.x.x #1

Closed
wants to merge 4 commits into from

Conversation

OnkarRuikar
Copy link

@OnkarRuikar OnkarRuikar commented Sep 28, 2023

Summary

The PR implements following things:

  • upgrade implementation to support webref/css v6.x.x
  • add support for other CSS features like function, type, at-rule, and at-rule-descriptor
  • update cli script
  • update readme
  • add testing mechanism
  • add tooling for linting and formatting

Implementation

Rather than using data provided by webref API as it is, it is better to extract the data as per our needs. This saves us from iterating over the same structure over and over for each API call.
All the data has been collected in seven hash maps for easy look ups. While populating the maps priority has been given to the "current" specs over the latest specs (with -x numbers). This makes the getSyntax() always return "current" syntax, and definitions of draft specs or part comes from the numbered specs.

Testing

For testing I've used npm pack command to create a local package query-css-syntax-0.0.4.tgz and used it in Yari to render {{CSSSyntax}} macros.
All the mdn/content documents built using yarn build at webref/css v5.4.4 were compared with this code at webref/css v6.7.1.
Apart from small corrections in syntaxes and new available syntaxex both the builds matched exactly.

Yari side code can be found in corresponding PR.

query-syntax.js Outdated Show resolved Hide resolved
@wbamberg
Copy link
Owner

wbamberg commented Oct 3, 2023

Another issue is that this ignores newValues: sometimes a thing is defined in one spec, then another spec defines some extra values under a newValues key. For example, position has its main definition in https://drafts.csswg.org/css-position/#position-property, but then https://drafts.csswg.org/css-gcpm-3/#running-syntax defines an additional value, and we want to have them included as well.

A lot of the details in how to assemble formal syntax was worked out in mdn/yari#4656, and the code that implements that is currently

/**
* If we have > 1 spec, assume that:
* - one of them is the base spec, which defines `values`,
* - the others define incremental additions as `newValues`
*
* Concatenate `newValues` onto `values` using `|`.
*/
function buildPropertySyntax(propertyName, specsForProp) {
let syntax = "";
let newSyntaxes = "";
for (const specName of specsForProp) {
const propertyData = parsedWebRef[specName].properties.filter(
(p) => p.name === propertyName
)[0];
const baseValue = propertyData.value;
if (baseValue) {
syntax = baseValue;
}
const newValues = propertyData.newValues;
if (newValues) {
newSyntaxes += ` | ${newValues}`;
}
}
// Concatenate newValues onto values to return a single syntax string
if (newSyntaxes) {
syntax += newSyntaxes;
}
return syntax;
}
.

@wbamberg
Copy link
Owner

wbamberg commented Oct 3, 2023

Also the logic for at-rule descriptors assumes that they are in a global namespace, but I don't think they are: for example the @media's aspect-ratio descriptor is defined separately from @container's aspect-ratio:

https://drafts.csswg.org/mediaqueries/#descdef-media-aspect-ratio
https://drafts.csswg.org/css-contain-3/#aspect-ratio

...the values are the same, but I don't see any reason that they have to be the same.

@OnkarRuikar OnkarRuikar marked this pull request as draft October 4, 2023 14:55
@OnkarRuikar
Copy link
Author

I've added small infrastructure for testing. Testing changes on yari is very painful. All the types and their syntaxes have been extracted and put under testing/data. To test changes in code we simply need to run npm t.

I've made changes for the values inside value issue.
Working on newValues issue.

Also the logic for at-rule descriptors assumes that they are in a global namespace, but I don't think they are: for example the @media's aspect-ratio descriptor is defined separately from @container's aspect-ratio

While testing on yari I found that some terms were defined in multiple specs. But in all the cases definitions/values were same. Could you give me an example where it's different? I mean if there is no case where it's different then we can go with this global namespacing right? Otherwise I'll update lookup logic for at-rule descriptors.

Also, consider a hypothetical case similar to this, if a user queries aspect-ratio directly then which one to pick? Should we make it mandatory to include the at-rule in at-rule-descriptor queries?
For example, node cli.js '@meida/aspect-ratio' 'at-rule-descriptor'.

@wbamberg
Copy link
Owner

wbamberg commented Oct 4, 2023

Also the logic for at-rule descriptors assumes that they are in a global namespace, but I don't think they are: for example the @media's aspect-ratio descriptor is defined separately from @container's aspect-ratio

While testing on yari I found that some terms were defined in multiple specs. But in all the cases definitions/values were same. Could you give me an example where it's different? I mean if there is no case where it's different then we can go with this global namespacing right? Otherwise I'll update lookup logic for at-rule descriptors.

Yeah, I don't know of any where it's different. But in the absence of any assurance the other way, if they are defined in two separate places, it seems safer to treat them as separate things. I wonder if it is worth asking the CSSWG about it - like, is it possible that the syntax of two at-rule descriptors with the same name might diverge?

Also, consider a hypothetical case similar to this, if a user queries aspect-ratio directly then which one to pick? Should we make it mandatory to include the at-rule in at-rule-descriptor queries? For example, node cli.js '@meida/aspect-ratio' 'at-rule-descriptor'.

Yes, I think we would need something like this.

@OnkarRuikar OnkarRuikar marked this pull request as ready for review October 6, 2023 08:58
@OnkarRuikar
Copy link
Author

Another issue is that this ignores newValues: sometimes a thing is defined in one spec, then another spec defines some extra values under a newValues key.

There is a flow in the old implementation. No same property object has both value and newValues at the same time. So I've put newValues processing at the end after collecting all the data.

Also, consider a hypothetical case similar to this, if a user queries aspect-ratio directly then which one to pick? Should we make it mandatory to include the at-rule in at-rule-descriptor queries? For example, node cli.js '@meida/aspect-ratio' 'at-rule-descriptor'.

Yes, I think we would need something like this.

I've implemented this. Now it is mandatory to use @font-face/unicode-range style descriptor names.

@OnkarRuikar
Copy link
Author

@wbamberg ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants