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

Display previous/next buttons and timer for inaccessible canvases #529

Merged
merged 5 commits into from
Jun 24, 2024

Conversation

Dananji
Copy link
Collaborator

@Dananji Dananji commented Jun 20, 2024

Related issue: #501

It was not possible to implement what was described in the done looks like in the ticket, as Video.js requires at least one source to build the player and its controls. And for inaccessible items the sources list is empty in the Canvas.

Therefore this UI was implemented, by wiring the same functions the previous/next buttons use in the player controls to the buttons in the display.

When first item is inaccessible only display the next button; (and only displays previous button when last item is inaccessible)
Screenshot 2024-06-24 at 3 38 36 PM

Previous and next buttons when item is in the middle of the list;
Screenshot 2024-06-24 at 3 39 24 PM

Hides the timer display when auto-advance is turned off;
Screenshot 2024-06-24 at 3 38 06 PM

@Dananji Dananji marked this pull request as draft June 20, 2024 15:24
@Dananji Dananji changed the title Timer display Display previous/next buttons and timer for inaccessible canvases Jun 20, 2024
@Dananji Dananji marked this pull request as ready for review June 20, 2024 15:51
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.

Looks good overall as far as I can tell.

If auto advance is turned off then should the timer countdown be hidden instead of dimmed?

src/components/MediaPlayer/MediaPlayer.js Outdated Show resolved Hide resolved
@Dananji
Copy link
Collaborator Author

Dananji commented Jun 24, 2024

I did think to hide the timer instead of dimming it and with the flexbox CSS it seemed to shift the content up and down when hiding and showing the timer. And I thought that behavior wouldn't be desirable so opted to dim the timer. But hiding the timer is a better option I can explore more on that.

@cjcolvar
Copy link
Member

What do you think of putting the countdown below the previous/next buttons? If it were below then there shouldn't be shifting from hiding/showing, right?

@Dananji Dananji force-pushed the timer-display branch 2 times, most recently from 99678a7 to 77cb321 Compare June 24, 2024 19:32
@Dananji
Copy link
Collaborator Author

Dananji commented Jun 24, 2024

Using visibility: hidden; instead of display: none; seems to fix the issue with shifting elements.

And I removed checkAutoAdvance param in switchPlayer() function as it is not used in the current code. This flag seemed to be used when we had switchPlayer() triggered within the custom VideoJS progress component. And this was removed recently with the Video.js refactor.

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.

Looks great!

@Dananji Dananji merged commit 94bde90 into main Jun 24, 2024
2 checks passed
@Dananji Dananji deleted the timer-display branch June 24, 2024 21:21
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.

2 participants