feat(FEC-13945): move core controls to upper bar #59
Conversation
lianbenjamin
requested review from
JonathanTGold,
SivanA-Kaltura,
semarche-kaltura and
MosheMaorKaltura
src/services/upper-bar-manager/ui/dropdown-bar/dropdown-bar.component.tsx
Outdated
Show resolved
Hide resolved
src/services/upper-bar-manager/ui/dropdown-bar/dropdown-bar.component.tsx
Show resolved
Hide resolved
src/services/upper-bar-manager/ui/dropdown-bar/dropdown-bar.component.tsx
Show resolved
Hide resolved
src/services/upper-bar-manager/ui/dropdown-bar-item/dropdown-bar-item.tsx
Show resolved
Hide resolved
| @@ -14,8 +14,9 @@ export class IconModel { | |||
| public componentRef: RefObject<IconWrapper>; | |||
| public onClick: (e: MouseEvent | KeyboardEvent) => void; | |||
| public component: ComponentClass<Record<string, never>> | FunctionalComponent<Record<string, never>>; | |||
| public svgIcon: SvgIcon; | |||
| public svgIcon: SvgIcon | (() => SvgIcon); | |||
There was a problem hiding this comment.
why it should be a function?
There was a problem hiding this comment.
the controls that I handled (PiP, CC, AAD, etc..) have 2 possible icons, which depends on the system's state. Hence, I added to the type a function that returns the SvgIcon.
| @@ -0,0 +1,58 @@ | |||
| import { KalturaPlayer, Logger, ui } from '@playkit-js/kaltura-player-js'; | |||
There was a problem hiding this comment.
if it is only for core controls, it is better call it something like core-controls-bars-mangerer
There was a problem hiding this comment.
hmmm not sure, i mean, right now it is for core controls but what if in the future it will be for others as well?
| @@ -27,6 +28,7 @@ export class IconModel { | |||
| this.componentRef = createRef(); | |||
| this.presets = | |||
| item.presets && item.presets.length > 0 ? item.presets : [ReservedPresetNames.Playback, ReservedPresetNames.Live]; | |||
| this.shouldHandleOnClick = typeof item.shouldHandleOnClick === 'boolean' ? item.shouldHandleOnClick : true; | |||
There was a problem hiding this comment.
why do we need this ? why just check if on click is supplied or not
There was a problem hiding this comment.
the new shouldHandleOnClick flag is indicating whether the upper bar should call the icon's onClick or not.
the reason is that the controls i handled to move to the upper bar (PiP, CC, AAD, etc..) are not only icons like the vamb plugin icons- they are actually the component itself; the way they behave in bottom bar- same in upper bar. which means, they have their own states and handlers.
another explanation:
let's say we don't have that flag, then the onClick will happen twice- 1 time from within the component and 1 time from the upper bar. Thus, the solution for this issue was to tell the upper bar- "I can handle myself, don't trigger my onClick".
There was a problem hiding this comment.
@lianbenjamin did you remove/change the old mechanisem (bottem bar manger) from core or you count on it ?
did not change the mechanism of the bottom bar- totally using it and counting on it. the only extra thing that the bottom bar is doing now, is updating a new redux state to keep the controls which will be removed from the bottom bar (and will be moved to the upper bar). |
### Description of the Changes **New requirement:** if there is not enough space for controls in bottom bar, instead of removing them- move them to the upper bar. This was requested for A11y purposes. - keep 10sec FWD and BWD controls in bottom bar (do not remove them) - new service to register relevant core components - new redux state to update the controls that need to be moved from bottom bar to upper bar - new interface that registers the components to the new service - a special implementation for `TimeDisplay` component (different behavior) related PR: kaltura/playkit-js-ui-managers#59 #### Resolves FEC-13945
Description of the Changes
following a new requirement to move core controls to the upper bar, in case there is not enough space for them:
Moredrop-down menu component:related PR- kaltura/playkit-js-ui#898
Solves FEC-13945
CheckLists