feat(controls): Add media settings control item components #1341
Conversation
src/lib/viewers/controls/media/MediaSettingsControls/MediaSettingsRadioItem.scss
Outdated
Show resolved
Hide resolved
src/lib/viewers/controls/media/MediaSettingsControls/MediaSettingsRate.tsx
Outdated
Show resolved
Hide resolved
jstoffan
force-pushed
the
controls-setting-2
branch
from
60dc5f8 to
49a249f
Compare
jstoffan
force-pushed
the
controls-setting-2
branch
from
49a249f to
4f6816d
Compare
| } | ||
| } | ||
|
|
||
| .bp-is-focused &:focus { |
There was a problem hiding this comment.
Is bp-is-focused needed? I don't see where that class is applied to the MediaSettingsMenuItem
There was a problem hiding this comment.
It's a holdover from the current experience in production where focus via keyboard has a different visual treatment than focus via mouse. The class is added by the MediaSettingsControls component.
| return; | ||
| } | ||
|
|
||
| setActiveMenu(target); |
There was a problem hiding this comment.
Do we need to worry about stopPropagation for these key events?
There was a problem hiding this comment.
Nope, we let them bubble up to be handled by MediaSettingsControls.
| <div className="bp-MediaSettingsRadioItem-check"> | ||
| <IconCheckMark24 className="bp-MediaSettingsRadioItem-check-icon" height={16} width={16} /> | ||
| </div> | ||
| <div className="bp-MediaSettingsRadioItem-value">{label || value}</div> |
There was a problem hiding this comment.
What is the purpose of having both label and value for this component?
There was a problem hiding this comment.
It will be relevant for the Autoplay menu, which shows the value true as "Enabled" to the user. The Rate menu shows most values as their own label, such as 1.5.
Continuing the changes mentioned in #1340. In my opinion, while these components are similar, they are different enough that sharing logic and/or markup between them would be somewhat awkward. I'm open to feedback and suggestions, though.