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 top level auto-advance toggle component. #218

Merged
merged 15 commits into from
Sep 1, 2023
Merged

Add top level auto-advance toggle component. #218

merged 15 commits into from
Sep 1, 2023

Conversation

cjcolvar
Copy link
Member

@cjcolvar cjcolvar commented Aug 18, 2023

image

Copy link
Contributor

@masaball masaball 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! Have a couple suggestions in the test suite, but nothing else jumped out.

@cjcolvar cjcolvar marked this pull request as ready for review August 29, 2023 17:06
@cjcolvar
Copy link
Member Author

The last change I made wires the toggle up to the manifest state and the player will check that when a canvas ends playing before moving on to the next canvas. This is a fairly significant change in behavior for the player as it autoplayed by default before. We could add auto-advance behavior to all of avalon's manifests but I'm not sure how hard that would be for other adopters. Maybe we could add a prop to IIIFPlayer to opt-in to this new behavior and deprecate the old behavior to be removed in a future major release?

@cjcolvar
Copy link
Member Author

There is a no-auto-advance behavior in the IIIF Presentation apec and it is supposed to be default. So I think we could consider Ramp's current behavior a bug. I'll go ahead and make the change for auto-advance to default to false unless it is present in the manifest. With this change it would probably be good to cut a major release and call out the change in behavior so adopters can modify their manifests accordingly.

@Dananji
Copy link
Collaborator

Dananji commented Aug 31, 2023

Looks good 💯
I had the following questions and suggestions, and one comment on the question about DRYing up the testing code above.

@Dananji
Copy link
Collaborator

Dananji commented Sep 1, 2023

This looks great 💯
I noticed just one thing below. After fixing it we can merge this 👍

src/components/IIIFPlayerWrapper.js Outdated Show resolved Hide resolved
@cjcolvar cjcolvar merged commit 1bdd8b9 into main Sep 1, 2023
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.

3 participants