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 animation-range support for ScrollTimeline #153

Merged
merged 20 commits into from
Jan 31, 2024

Conversation

bramus
Copy link
Collaborator

@bramus bramus commented Sep 13, 2023

This PR adds support for animation-range for ScrollTimeline based SDAs. It is done by parsing the animation range (with a fallback to the defaults of 0% and 100%) and then calculating the relative ScrollTimeline position. This flow has been copied over from how ViewTimelines are being handled.

It also includes a little refactoring that splits off normalizeAxis into its own function, as the logic to determine that ended up being present 3 times in the source code.

Fixes #151.

@bramus bramus changed the title Feat scroll timeline animation range Add animation-range support for ScrollTimeline Sep 13, 2023
src/proxy-animation.js Outdated Show resolved Hide resolved
@bramus
Copy link
Collaborator Author

bramus commented Sep 13, 2023

Addressed the remarks (+ merge conflict), but am wondering: maybe I should also move the parseAnimationRange functions onto the classes as statics?

@ryantownsend
Copy link

+1 for this issue – great to see the PR @bramus

My usage includes viewport units which I can see aren't included in the PR, would that be straight-forward to introduce? I'm not asking you to do it, I'm happy to contribute if I can, I'm just unfamiliar with the codebase – I just figure the responsive nature of viewport units could be tricky if the values are evaluated on discovery rather than in 'real time'.

@bramus
Copy link
Collaborator Author

bramus commented Sep 14, 2023

@ryantownsend I once started a PR to add viewport units – #52 – but that one has stalled. Haven’t found time to further look into it, but feel free to pick it up. It’s quite an old PR, so does need a rebase …

@kevers-google
Copy link
Collaborator

Hi Ryan,
If you would like to contribute, I'll be happy to review the code. Alas, I'm a bit short on free cycles, or I'd dive in and help tidy things up.

@ryantownsend
Copy link

Thanks @bramus / @kevers-google – I'll see if I get some time over the next week or so.

@bramus bramus marked this pull request as draft October 6, 2023 14:04
@bramus
Copy link
Collaborator Author

bramus commented Oct 6, 2023

Converting to draft as I noticed a bug in the calculations … need to dig into that.

@bramus
Copy link
Collaborator Author

bramus commented Dec 21, 2023

FYI: I’m awaiting some of the refactors from @johannesodland’s PRs before picking this one up again.

@bramus
Copy link
Collaborator Author

bramus commented Dec 26, 2023

I think I re-landed the changes of the original branch on top of master. As I suspected, the issue with the wrong range being applied got fixed thanks to #176 (confirmed through manual testing)

The tests, however, don’t seem to have made a huge jump: only 2 extra tests pass. Also seeing timeouts for the tests with the timeline-range … that needs to be looked into.

@bramus bramus marked this pull request as ready for review December 26, 2023 00:30
@bramus bramus requested review from flackr and removed request for kevers-google December 26, 2023 00:38
@bramus bramus dismissed kevers-google’s stale review December 26, 2023 18:20

Changes have been included while re-landing this commit.

@bramus bramus requested a review from kevers-google December 26, 2023 18:20
src/scroll-timeline-base.js Outdated Show resolved Hide resolved
src/scroll-timeline-base.js Outdated Show resolved Hide resolved
@bramus bramus force-pushed the feat-scroll-timeline-animation-range branch from be4645b to bdc43e4 Compare December 30, 2023 20:38
@bramus bramus force-pushed the feat-scroll-timeline-animation-range branch from e83cca4 to f011e3b Compare January 3, 2024 22:03
package.json Outdated Show resolved Hide resolved
src/scroll-timeline-base.js Outdated Show resolved Hide resolved
src/proxy-animation.js Outdated Show resolved Hide resolved
@bramus bramus force-pushed the feat-scroll-timeline-animation-range branch from f011e3b to a596e06 Compare January 26, 2024 23:01
@bramus bramus requested review from johannesodland and flackr and removed request for kevers-google January 26, 2024 23:25
@bramus
Copy link
Collaborator Author

bramus commented Jan 26, 2024

Addressed all remarks. Requesting a new review, @flackr @johannesodland

Copy link
Contributor

@johannesodland johannesodland left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I've added some comments with regards to #209 that adds a function to split a string into component values.

If we merge #209 first we should use that function here, otherwise I can include those changes in #209.

package.json Outdated Show resolved Hide resolved
src/proxy-animation.js Outdated Show resolved Hide resolved
src/proxy-animation.js Show resolved Hide resolved
src/proxy-animation.js Show resolved Hide resolved
src/proxy-animation.js Show resolved Hide resolved
Copy link
Owner

@flackr flackr left a comment

Choose a reason for hiding this comment

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

Looks good.

src/proxy-animation.js Outdated Show resolved Hide resolved
@bramus bramus merged commit b2f44df into flackr:master Jan 31, 2024
2 checks passed
@bramus bramus deleted the feat-scroll-timeline-animation-range branch February 4, 2024 07:47
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.

Is animation-range for scroll-timeline not supported?
5 participants