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 previous/next section buttons to player icons #188

Merged
merged 1 commit into from
May 30, 2023
Merged

Conversation

Dananji
Copy link
Collaborator

@Dananji Dananji commented May 24, 2023

Screenshot from 2023-05-24 14-09-00

Closes #176

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

This looks great! The only question I had was if the previous/next buttons should only appear when there are previous or next canvases to change to? I noticed that when on the first canvas the previous button sets the player to the beginning of the section while the next button on the last canvas does nothing. Maybe these buttons could be rendered but disabled when they aren't actionable? But if how you coded things is the desired behavior then feel free to go ahead and merge.

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

This PR changes locations of some files. #177 modifies some of the same files, does it need to be merged first?

@Dananji
Copy link
Collaborator Author

Dananji commented May 30, 2023

The only question I had was if the previous/next buttons should only appear when there are previous or next canvases to change to?

Yes, previous/Next buttons are displayed only when there are canvases to switch

I noticed that when on the first canvas the previous button sets the player to the beginning of the section while the next button on the last canvas does nothing. Maybe these buttons could be rendered but disabled when they aren't actionable?

When on first canvas the previous button restarts playback from start and it is conveyed in the title of the button when hovered so that the user is not confused. This was unusual to me at first because I haven't paid attention to this before, but this seems to be the common behavior of major players out there :)
As for the last canvas Jon's expectation was that the next button does nothing, but I like the idea of disabling it since it doesn't do anything in that use-case. Maybe we could revisit this in the future 👍

@Dananji Dananji merged commit f72bd05 into main May 30, 2023
@Dananji Dananji deleted the section-buttons branch May 30, 2023 20:46
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.

Add Previous and Next Buttons to navigate between Canvases
2 participants