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.

Sign up for GitHub

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

Jump to bottom

feat(UI): Add the option for chapter titles and dividers on the seek bar #5771

Closed
wants to merge 15 commits into from
Closed

feat(UI): Add the option for chapter titles and dividers on the seek bar #5771

wants to merge 15 commits into from

Conversation

Trail3lazer
Copy link

@Trail3lazer Trail3lazer commented Oct 14, 2023 •
edited by avelad

Closes #3597

@Trail3lazer Trail3lazer mentioned this pull request Oct 14, 2023
Open
@Trail3lazer Trail3lazer changed the title Add the option for chapter titles and dividers on the seek bar feat: Add the option for chapter titles and dividers on the seek bar Oct 14, 2023
@avelad avelad changed the title feat: Add the option for chapter titles and dividers on the seek bar feat(UI): Add the option for chapter titles and dividers on the seek bar Oct 14, 2023
@avelad avelad added type: enhancement New feature or request component: UI The issue involves the Shaka Player UI priority: P3 Useful but not urgent labels Oct 14, 2023
@avelad avelad added this to the v4.6 milestone Oct 14, 2023
@avelad
Copy link
Collaborator

avelad commented Oct 14, 2023

@Trail3lazer I’m on vacation the next week, but I’ll review when back! Thanks!

@shaka-bot
Copy link
Collaborator

shaka-bot commented Oct 14, 2023 •
edited

Incremental code coverage: 35.95%

@absidue
Copy link

absidue commented Oct 15, 2023

Looks like you've tied the chapters to the current subtitle/caption language, how will that work if the video doesn't have any subtitles/captions or when they are disabled or if the chapters are only available in a single language but a subtitle track in a different language is active?

Probably better to either hook it up to the UI language which will always be available or create a separate setting for it.

avelad
avelad requested changes Oct 23, 2023
View reviewed changes
docs/tutorials/ui-customization.md
@@ -96,6 +96,7 @@ The following buttons can be added to the overflow menu:
* remote: adds a button that opens a Remote Playback dialog. The button is visible only if the
browser supports Remote Playback API.
* Statistics: adds a button that displays statistics of the video.
* displayChapters: tick marks between chapters and labels that appear on hover.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This configuration does not have to exist, in the same way as we do with thumbnails. Please, remove this config.

docs/tutorials/ui-customization.md
@@ -171,7 +172,7 @@ const config = {
'seekBarColors': {
base: 'rgba(255, 255, 255, 0.3)',
buffered: 'rgba(255, 255, 255, 0.54)',
played: 'rgb(255, 255, 255)',
played: 'rgb(255, 255, 255)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert this change, it's not necessary

test/ui/ui_unit.js
@@ -880,7 +880,6 @@ describe('UI', () => {
});
});


Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert this change, it's not necessary

ui/seek_bar.js Show resolved Hide resolved
ui/seek_bar.js Show resolved Hide resolved
ui/seek_bar.js Show resolved Hide resolved
ui/seek_bar.js Show resolved Hide resolved
ui/seek_bar.js Show resolved Hide resolved
ui/seek_bar.js
@@ -607,7 +799,6 @@ shaka.ui.SeekBar = class extends shaka.ui.RangeElement {
}
};


Copy link
Collaborator

Choose a reason for hiding this comment

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

Revert this change, it's not necessary

ui/seek_bar.js Show resolved Hide resolved
@avelad avelad added the status: waiting on response Waiting on a response from the reporter(s) of the issue label Oct 26, 2023
@shaka-bot
Copy link
Collaborator

Closing due to inactivity. If the author would like to continue this PR, they can reopen it (preferred) or start a new one (if needed).

@shaka-bot shaka-bot closed this Nov 2, 2023
@shaka-bot shaka-bot removed the status: waiting on response Waiting on a response from the reporter(s) of the issue label Nov 2, 2023
@Trail3lazer
Copy link
Author

Trail3lazer commented Nov 11, 2023 •
edited

@avelad, I removed the changes you recommended and the unnecessary configuration value. Sorry, I was unavailable to update this until now. Did you merge the changes already with a different PR?

edit: In this PR: #5863

@Trail3lazer Trail3lazer mentioned this pull request Nov 11, 2023
Merged
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Jan 1, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Jan 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@avelad avelad avelad requested changes

Assignees
No one assigned
Labels
component: UI The issue involves the Shaka Player UI priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
v4.6
Development

Successfully merging this pull request may close these issues.

Add chapter marks in the seek bar
4 participants
@Trail3lazer @avelad @shaka-bot @absidue