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

feat(ui): Add quality selection UI for audio-only content #1

Draft
wants to merge 3 commits into
base: feat-2071
Choose a base branch
from

Conversation

nbcl
Copy link
Owner

@nbcl nbcl commented Apr 23, 2021

Context

This is a draft pull request that is currently under development.

Description

I approached the issue by introducing the quality selection to the currently existing resolution button in the UI, conditionally, when audio-only content is being displayed.

tracks = tracks.filter((track, idx) => {
  // If audio-only content is being displayed
  if (this.player.isAudioOnly()) {
    // Keep the first one with the same audioBandwidth.
    otherIdx = tracks.findIndex(
        (t) => t.audioBandwidth == track.audioBandwidth);
  // If video content is being displayed
  } else {
    // Keep the first one with the same height.
    otherIdx = tracks.findIndex(
        (t) => t.height == track.height);
  }
  return otherIdx == idx;
});

Currently, this replaces the resolution menu with a quality menu when audio-only content is displayed, as shown in the following image:

Draft status

Currently, the following points must be decided before resolving the draft.

  • Make sure the obtaining of the audio quality attribute is correct.
  • Decide if a new button must be created to only handle audio quality or having the resolution button shift functionality depending on content is the correct design.

this.backSpan.textContent =
// Assign text attributes depending on content type.
if (this.player.isAudioOnly()) {
this.button.setAttribute(shaka.ui.Constants.ARIA_LABEL,

Choose a reason for hiding this comment

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

Looks like you could set a local variable with the localization ID, instead of duplicating all the setAttribute and resolve calls in each branch.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Of course, that works way better - Updated it on my last commit!

// Assign text attributes depending on content type.
const locId = this.player.isAudioOnly() ? LocIds.QUALITY : LocIds.RESOLUTION;

For audio-only content, in the UI, the resolution button becomes a quality button where users can select their desired audioBandwidth.

Issue: shaka-project#2071
Replaces "if" statement that duplicated setAttribute and textContent declarations with a const defined by a conditional operator.
Switches to bandwidth from audioBandwith in the audio quality selection version of the resolution button to prevent cases in which audioBandwith may not be present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants