-
Notifications
You must be signed in to change notification settings - Fork 73
feat(packages): add time display toggle #1669
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ export interface TimeProps { | |
| negativeSign?: string | undefined; | ||
| /** Custom label for accessibility. */ | ||
| label?: string | ((state: TimeState) => string) | undefined; | ||
| /** Whether the time display can be toggled. */ | ||
| toggle?: boolean | undefined; | ||
| } | ||
|
|
||
| export interface TimeState { | ||
|
|
@@ -32,17 +34,18 @@ export interface TimeState { | |
| datetime: string; | ||
| } | ||
|
|
||
| const DEFAULT_LABELS: Record<TimeType, string> = { | ||
| current: 'Current time', | ||
| duration: 'Duration', | ||
| remaining: 'Remaining', | ||
| const TOGGLE_LABELS: Record<TimeType, string> = { | ||
| current: 'Show elapsed time', | ||
| duration: 'Show duration', | ||
| remaining: 'Show remaining time', | ||
| }; | ||
|
Comment on lines
+37
to
+41
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume this would complicate the i18n work. I think the ARIA attributes are already indicating that this can be pressed/toggled. YT seems to only require the addition of role and tabIndex. Ideally we can remove.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've asked in the shared a11y about the best pattern here. I don't think YouTube has this quite right as there's no indication of what the button actually does, the label only informs of the current state. I could be wrong but waiting on clarification. I think we want the current state and next state to be obvious to screen readers. |
||
|
|
||
| export class TimeCore { | ||
| static readonly defaultProps: NonNullableObject<TimeProps> = { | ||
| type: 'current', | ||
| negativeSign: '-', | ||
| label: '', | ||
| toggle: false, | ||
| }; | ||
|
|
||
| #props = { ...TimeCore.defaultProps }; | ||
|
|
@@ -98,7 +101,15 @@ export class TimeCore { | |
| return secondsToIsoDuration(Math.abs(seconds)); | ||
| } | ||
|
|
||
| getLabel(state: TimeState): string { | ||
| #getToggleType(type: TimeType, currentType: TimeType): TimeType { | ||
| if (type === 'current') { | ||
| return currentType === 'remaining' ? 'current' : 'remaining'; | ||
| } | ||
|
|
||
| return currentType === 'duration' ? 'remaining' : 'duration'; | ||
| } | ||
|
|
||
| getLabel(state: TimeState, type = this.#props.type): string { | ||
| const { label } = this.#props; | ||
|
|
||
| if (isFunction(label)) { | ||
|
|
@@ -108,13 +119,20 @@ export class TimeCore { | |
| return label; | ||
| } | ||
|
|
||
| return DEFAULT_LABELS[this.#props.type]; | ||
| if (!this.#props.toggle) { | ||
| return state.phrase; | ||
| } | ||
|
|
||
| const toggleType = this.#getToggleType(type, state.type); | ||
|
|
||
| return `${state.phrase}. ${TOGGLE_LABELS[toggleType]}.`; | ||
| } | ||
|
|
||
| getAttrs(state: TimeState) { | ||
| getAttrs(state: TimeState, type = this.#props.type) { | ||
| return { | ||
| 'aria-label': this.getLabel(state), | ||
| 'aria-valuetext': state.phrase, | ||
| 'aria-label': this.getLabel(state, type), | ||
| role: this.#props.toggle ? 'button' : undefined, | ||
| tabIndex: this.#props.toggle ? 0 : undefined, | ||
| }; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,7 +89,7 @@ function getTemplateHTML() { | |
|
|
||
| <div class="${time.controls}"> | ||
| <media-time-group class="${time.group}"> | ||
| <media-time type="current" class="${time.current}"></media-time> | ||
| <media-time toggle type="current" class="${time.current}"></media-time> | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only added it to the minimal skin as the default skin felt a bit better with the remaining in place of the duration. We'll revisit this before GA though I guess as we may want to make minimal a bit more minimal in terms of UI. I'm just waiting for the menus stuff to merge so we're reasonably feature stable for the skins. |
||
| <media-time-separator class="${time.separator}"></media-time-separator> | ||
| <media-time type="duration" class="${time.duration}"></media-time> | ||
| </media-time-group> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.