add autostart button to videoplayer#6368
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Still working on this PR. I planned on providing screenshots once I've polished the styling |
|
I've gotten the CSS to a point I'm happy with. Here are pictures. Acfive state: Disabled state: I would like to eventually create sections within the settings button where buttons such as the playback options can be tucked into. But that's a separate conversation. I'll address the remaining linter issues tomorrow. |
|
I like it. I have 2 questions.
|
|
This PR should be ready for a review now.
Yes, this new button also display descriptive text on hover. Trying to capture a screenshot of this while keep the text on screen proved difficult but here is the code: The
Yes, the code will sync all toggle actions between the video player button as well as settings interface button. |
WithoutPants
left a comment
WithoutPants
left a comment
There was a problem hiding this comment.
The styling needs to be adjusted for viewports between 577px and 700px. The cog icon and full screen buttons end up chopped off:
|
|
||
| if (this.autostartEnabled) { | ||
| this.addClass("vjs-icon-play-circle"); | ||
| this.controlText(this.localize("Auto-start enabled (click to disable)")); |
There was a problem hiding this comment.
Could we accept localised strings in the constructor options instead? That way we could put the strings into the localisation files.
There was a problem hiding this comment.
This was interesting. I saw we did localization this way for some of the other VideoJS buttons. I kept the approach for consistency, but I could add a localised string here if you don't mind the inconsistency.
There was a problem hiding this comment.
Fair point. Leave it as is for consistency. We should address localisation in another PR.
What are your thoughts on hiding the auto-play button for now on mobile until button consolidation has been done? The button overload is more apparent on mobile |
Sounds good |
| } | ||
|
|
||
| // make controls a little more compact on smaller screens | ||
| @media (max-width: 576px) { |
There was a problem hiding this comment.
The buttons are still currently overflowing offscreen between 576px and ~676px width. I reckon it would make sense to change this clause to (max-width: 768px) and move the hide autostart button to a separate (max-width: 576px) clause.
This is with that fix at 600px width:
There was a problem hiding this comment.
Sorry, I'm just seeing your message. I'm currently unable to test the change, but I've applied the update as you've mentioned.
hopeful linter fix
3 participants
This pull request adds the auto-start button, which was previously behind the interface settings, directly to the video player, similar to most modern video players.