-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Sync css.types.attr with CSS Values 5 extensions
#26624
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
Conversation
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
b7ac092 to
d4ea9c4
Compare
|
@hamishwillee Would you mind taking a look at these changes? 🙏 |
9c31c5d to
eccf31e
Compare
Changes: - Removed `type-or-unit` subfeature: `frequency`. - Renamed `fallback` to `fallback-value`. - Renamed `type-or-unit` to `type_function`. - Wrapped all keywords in fences. - Added subfeatures: `custom-ident`, `ident`, `image`, `length-percentage`, `resolution`, `string`, `transform-function`.
eccf31e to
7060070
Compare
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.
@caugner Sorry, I missed this request for review - I rarely get asked for reviews here, so worth pinging me directly if I can be of use.
The information seems correct, but I've added some queries on process such as https://github.com/mdn/browser-compat-data/pull/26624/files#r2214505721 that essentially ask:
- Have the removed features been gone for more than two years - i.e. is it right to remove them.
- Does it matter that the name of a property does not match the spec https://github.com/mdn/browser-compat-data/pull/26624/files#r2214505721 if it matches the docs
- How will linking to the
type_functionhappen? Note, I've added mdn/content#40418 to create DL links that could work. Should we manually add links.
I'm going to approve but not merge.
@chrisdavidmills You reviewed the docs. Since @caugner is heading away for a few weeks, can you answer/confirm my questions here?
4a407dc to
ac88b42
Compare
| ], | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": false |
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.
My interpretation of chromium/chromium@01b72da is that <ident> was not supported in Chrome 133.
| ], | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": false |
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.
My interpretation of chromium/chromium@01b72da is that <image> was not supported in Chrome 133.
| ], | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": false |
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.
My interpretation of chromium/chromium@01b72da is that <length-percentage> was not supported in Chrome 133.
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.
If <length> is supported and <percentage> is supported, what does it mean for <length-percentage> to not be supported?
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.
If I understand this correctly, then <length-percentage> is a combination (not a union) of <length> and <percentage>.
It seems to be somewhat similar to https://github.com/search?q=repo%3Amdn%2Fbrowser-compat-data+%2Fmixed_type_parameters%2F&type=code.
Let's merge as is, and follow up separately if need be.
| ], | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": false |
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.
My interpretation of chromium/chromium@01b72da is that <resolution> was not supported in Chrome 133.
| ], | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": false |
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.
My interpretation of chromium/chromium@01b72da is that <transform-function> was not supported in Chrome 133.
|
@hamishwillee Could you please take another last look? 🙏 |
| "number": { | ||
| "__compat": { | ||
| "description": "<number>", | ||
| "description": "`<number>`", |
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.
We should also add a separate number keyword.
e.g. attr(data-foo number)
See: https://chromiumdash.appspot.com/commit/def86f45b9de19ae59340f0e73cb0c50da78421e
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.
| } | ||
| }, | ||
| "type-or-unit": { | ||
| "type_function": { |
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.
<attr-type> = type( <syntax> ) | raw-string | number | <attr-unit>
Should we add a separate <attr-unit> type?
e.g. attr(data-foo px)
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.
We can add that later if is there is evidence that support for different units were added at different types.
To distinguish it from keywords in the e.g. |
ddbeck
left a comment
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.
This looks plausible to me. One question in a line comment, but I'm happy to see this merge.
| ], | ||
| "support": { | ||
| "chrome": { | ||
| "version_added": false |
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.
If <length> is supported and <percentage> is supported, what does it mean for <length-percentage> to not be supported?
Summary
Changes:
Removedtype-or-unitsubfeature:frequency.fallbacktodeclaration-value.type-or-unittotype_function, adding subfeatures:<custom-ident>,<ident>,<image>,<length-percentage>,<resolution>,<string>,<transform-function>.Test results and supporting details
Related issues
Fixes #25618.