-
Notifications
You must be signed in to change notification settings - Fork 642
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
[scroll-animations-1] Describe scroll/view-timeline-attachment #8680
Conversation
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.
Overall looks good, but I think if we can't allow axis on the ancestor
attachment shorthand that we should just leave the shorthand general even though the axis on the 'defer' attachment isn't used.
scroll-animations-1/Overview.bs
Outdated
### Scroll Timeline Shorthand: the 'scroll-timeline' shorthand ### {#scroll-timeline-shorthand} | ||
|
||
<pre class='propdef shorthand'> | ||
Name: scroll-timeline | ||
Value: [ <<'scroll-timeline-name'>> <<'scroll-timeline-axis'>>? ]# | ||
Value: [ <<'scroll-timeline-name'>> [ <<'scroll-timeline-axis'>> | <<'scroll-timeline-attachment'>> ] ? ]# |
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 makes it impossible for the descendent to declare the axis unfortunately. In order to avoid an axis specified on the deferred attachment It would have to be split up into that value which I'm not sure is something we do. e.g.
Value: [ <<'scroll-timline-name'>> 'defer' | <<'scroll-timline-name'>> <<'scroll-timeline-axis'>>? 'ancestor'? ]#
I'm fine with just doing the simple thing and having them all in the shorthand if this isn't speccable.
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.
Ah, yes, good point. Fixed (did the simple thing for now).
Although something like what you said does seem speccable. "Splitting" the value to achieve something special doesn't seem too far off from what we're doing in e.g. https://drafts.csswg.org/css-text-4/#white-space-property.
So maybe we could do: <<'scroll-timeline-name'>> [ defer | [ <<'scroll-timeline-axis'>> ancestor? ] ]?
But maybe we leave this for a separate PR, for maximum bikeshedding.
Looks good. Thank you, Anders. |
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.
Looks good 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.
@andruud I know I'm asking for a bit of a conceptual rewrite... if you prefer for me to draft it up for you, just lmk, and I'll add it as a commit on this branch. (I was planning to work on this today anyway.)
@fantasai If you are dead set on "scope a name", then yes please, as I'm not sure how to actually spec that. No point in me trying to guess what you would have written. :-) See other comment, though. |
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.
Some review comments on what you've got so far.
Pushed a commit with some minor clean-ups. (I'll try to figure out filing a PR against a PR for the name-scoping bit, though, so you can review it. :) |
@andruud I opened andruud#1 to recast this in terms of a name binding. There are some additional fixes and clarifications there, so if we end up going with this base version I'll need to port them over; but I didn't want to force-rebase on your branch or create multiple merge-conflicting PRs, so... it's a bit messy on that point. :/ The end result should at least be pretty readable? |
CC @tabatkins for opinions... |
Thanks @fantasai, that looks good to me if we can eliminate the circularity there. :-) |
@andruud I committed a bunch of editorial improvements to try to address @tabatkins's review that neither of our versions was very understandable. :) Hoping this is good to land now (but we should DEFINITELY squash the history into a single co-authored commit). |
@fantasai This is very clean now to my eyes. (But of course I'm tainted by already knowing how it should work :-)).
For sure. :-) |
I missed some of this in the last commit.
@andruud OK, merged. Thanks for working on this with me! |
Issue #7759.