-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Improve startViewTransition() data #28541
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
base: main
Are you sure you want to change the base?
Improve startViewTransition() data #28541
Conversation
|
Tip: Review these changes grouped by change (recommended for most PRs), or grouped by feature (for large PRs). |
caugner
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.
LGTM overall, but the nested parameters should not be nested according to BCD guidelines.
| "types_property": { | ||
| "__compat": { | ||
| "description": "`types` property", |
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.
BCD guidelines suggest to not nest these:
| "types_property": { | |
| "__compat": { | |
| "description": "`types` property", | |
| "options_types_parameter": { | |
| "__compat": { | |
| "description": "`options.types` parameter", |
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.
/cc @ddbeck Is this a special case warranting an exception? (I don't remember understanding why we don't nest these.)
Edit: Nesting has the benefit of linting the child support data against the parent support data.
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.
Yeah, not nesting them seems a little odd to me.
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.
Honestly, I think the reason we had that guideline was that deeply-nested keys didn't get rendered on compat tables because of recursion limit on MDN. I'm not sure what the limit is today or if it still matters.
I agree that having the linting on nested data is a real gain. I'm fine with deviating from the guideline for this PR. And we ought to consider formally revising the existing guideline. @caugner would you mind putting it on the agenda for the first BCD call in January?
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 reason we had that guideline was that deeply-nested keys didn't get rendered on compat tables because of recursion limit on MDN. I'm not sure what the limit is today or if it still matters.
Nowadays, the BCD table hides nested features only there would be too many features otherwise: https://github.com/mdn/fred/blob/7d1da1e38e4394f5fd04b4d8c5c90073ee4b0134/components/compat-table/element.js#L345-L348
So I agree with deviating from the guideline for this PR, and discuss updating it in January.
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.
That said, I think the description should still reflect the full path (and use the term "parameter", not "property"):
| "types_property": { | |
| "__compat": { | |
| "description": "`types` property", | |
| "types_parameter": { | |
| "__compat": { | |
| "description": "`options.types` parameter", |
| "update_property": { | ||
| "__compat": { | ||
| "description": "`update` property", |
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.
| "update_property": { | |
| "__compat": { | |
| "description": "`update` property", | |
| "update_parameter": { | |
| "__compat": { | |
| "description": "`options.update` parameter", |
Edit: Updated following #28541 (comment).
|
As an FYI, I've updated the Firefox support for these updates to "preview": see https://bugzilla.mozilla.org/show_bug.cgi?id=2001878. Update: After more testing, I'm pretty sure it should just be "147", not "preview". There doesn't appear to be a pref to flip for this. |
Summary
Chrome and Safari have both supported view transition types (see https://chromestatus.com/feature/5089552511533056) for a while, so it is high time I got them documented on MDN.
Having checked BCD, it seems that we have pretty much all of the data we need in place already, but I wanted to improve the
Document.startViewTransition()data:a.
updateCallbackmakes sense — this is the callback parameter mentioned in 1.b.
callbackOptionsdoesn't make sense. This refers to a mixin in the spec that represents the possible parameter values as one item. This is a spec construct and not a real value that developers can use. It would be better to have a sub-data point for the options object, with sub-sub-data points for the possible object properties. This last bit is maybe not totally necessary, but I thought it would be useful for developers who are specifically checking the compat table fortypessupport.This PR updates the data as described above, and also updates the spec URLs so they all point to the Level 2 of the spec
Test results and supporting details
Related issues