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

Added 'title' attribute to the audio track button #3565

Conversation

jbarabander
Copy link
Contributor

@jbarabander jbarabander commented Aug 24, 2016

Description

Fixes #3528.
The audio track button on the video player was missing the 'title' attribute due to the underlying component not having a controlText_ property.

Specific Changes proposed

Assigned controlText_ property of AudioTrackButton to 'Audio'.

Requirements Checklist

  • Feature implemented / Bug fixed
  • Reviewed by Two Core Contributors

@jbarabander jbarabander changed the title Added controlText_ to AudioTrackButton Added 'title' audio track button Aug 24, 2016
@jbarabander jbarabander changed the title Added 'title' audio track button Added 'title' to audio track button Aug 24, 2016
@jbarabander jbarabander changed the title Added 'title' to audio track button Added 'title' attribute to audio track button Aug 24, 2016
@jbarabander jbarabander changed the title Added 'title' attribute to audio track button Added 'title' attribute to the audio track button Aug 24, 2016
@brandonocasey
Copy link
Contributor

LGTM

@gkatsev gkatsev added the patch This PR can be added to a patch release. label Aug 24, 2016
@dmlap
Copy link
Member

dmlap commented Aug 24, 2016

LGTM

1 similar comment
@gkatsev
Copy link
Member

gkatsev commented Aug 24, 2016

LGTM

@mister-ben
Copy link
Contributor

Could "Audio" be confused with the volume controls? "Audio track"?

@jbarabander
Copy link
Contributor Author

@mister-ben: I did indeed consider this. I opted for 'Audio' because it seemed generally consistent with other controlText_ (namely most of the other titles for the buttons were one word) and I didn't personally find it super confusing. However I'd be glad to change it to 'Audio Track' if you guys deem that that would be better.

@gkatsev
Copy link
Member

gkatsev commented Aug 24, 2016

Audio is also consistent with the other menu buttons we have like captions and subtitles.

gkatsev added a commit to gkatsev/video.js that referenced this pull request Aug 24, 2016
@gkatsev
Copy link
Member

gkatsev commented Aug 24, 2016

Realized we should update the en.json file: #3569

@gkatsev
Copy link
Member

gkatsev commented Aug 24, 2016

I wonder if "Audio Track" would be confusing for people not invested in video players and the technology being used.

@brandonocasey
Copy link
Contributor

brandonocasey commented Aug 24, 2016

Perhaps "Audio Selection"? Audio To me seems to imply that you will be able to edit or view settings on the current audio track.

@heff
Copy link
Member

heff commented Aug 24, 2016

I think Track is a common enough word. YouTube appears to use it in their docs a number of places at least.

@mmcc
Copy link
Member

mmcc commented Aug 24, 2016

Yeah "Audio Track" feels like a nice compromise. Especially given this is accessibility related, I don't think there would be confusion as to what this means.

@gkatsev
Copy link
Member

gkatsev commented Aug 24, 2016

Sounds like most people would prefer "Audio Track", I'm ok with that too.

@jbarabander would you be up for making the change? Either as another commit on this branch or if you can do a PR based on the stable branch, that would be awesome. If you want to incorporate #3569 into it, would be great too.

@jbarabander
Copy link
Contributor Author

@gkatsev Sure thing! I'll make a new PR based off the stable branch and I'll wrap #3569 into it as well.

@gkatsev
Copy link
Member

gkatsev commented Aug 24, 2016

Closing this PR then, since it's already been merged to stable and we'll have a new updated PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed patch This PR can be added to a patch release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'title' attribute doesn't appear for audio button
7 participants