-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Added 'title' attribute to the audio track button #3565
Conversation
LGTM |
LGTM |
1 similar comment
LGTM |
Could "Audio" be confused with the volume controls? "Audio track"? |
@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. |
Audio is also consistent with the other menu buttons we have like |
Realized we should update the en.json file: #3569 |
I wonder if "Audio Track" would be confusing for people not invested in video players and the technology being used. |
Perhaps "Audio Selection"? Audio To me seems to imply that you will be able to edit or view settings on the current audio track. |
I think Track is a common enough word. YouTube appears to use it in their docs a number of places at least. |
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. |
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. |
Closing this PR then, since it's already been merged to stable and we'll have a new updated PR. |
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