feat(external-api) extend event to listen to system buttons and add config to prevent execution #13529
Conversation
mihhu
force-pushed
the
participant-menu-button-click
branch
5 times, most recently
from
70d776c
to
6e816ca
Compare
mihhu
force-pushed
the
participant-menu-button-click
branch
from
6e816ca
to
36b7777
Compare
…onfig to prevent execution
mihhu
force-pushed
the
participant-menu-button-click
branch
from
36b7777
to
a4c97c7
Compare
| APP.API.notifyToolbarButtonClicked( | ||
| buttonKey, notifyMode === NOTIFY_CLICK_MODE.PREVENT_AND_NOTIFY | ||
| ); | ||
| if (notifyClick) { |
There was a problem hiding this comment.
This reads weird. Why does one type of button notify one way and the other a different wat? Matybe we need some notifyType to discriminate?
There was a problem hiding this comment.
There are only a handful of participant context menu buttons that extend AbstractButton. I thought the ideal implementation would be to refactor those and leave the toolbar buttons have their own notification system implemented here. The number of toolbar buttons and their instantiations is quite intimidating and I'm afraid I'd miss something for sure. I would've went for the ideal if the PR wasn't already so verbose 🥲, wdyt?
There was a problem hiding this comment.
IMHO this makes things worse in the sense that there is more code to remove after we do things The Right Way (TM). So we are adding to the debt too.
AbstractButton should've been called AbstractToolbarButton but being part of the toolbox feature kind of explains that, oh well.
I'd say let's not have buttons derived from there unless they are toolbar buttons. There is where the complexity starts.
There was a problem hiding this comment.
@robertpin Can you weigh in here too?
There was a problem hiding this comment.
I'd say let's not have buttons derived from there unless they are toolbar buttons.
I noticed that the buttons that aren't extending the AbstractButton class on the web are extending it on the native side. Should I refactor everything? Or should I leave native for another PR?
react/features/video-menu/components/web/HideSelfViewVideoButton.tsx
Outdated
Show resolved
Hide resolved
react/features/video-menu/components/web/TogglePinToStageButton.tsx
Outdated
Show resolved
Hide resolved
|
@robertpin I'd like your input on this one please. |
I think @horymury worked on this in the past so he is better suited for this one |
mihhu
force-pushed
the
participant-menu-button-click
branch
from
2ab5a3f
to
d30fe41
Compare
mihhu
force-pushed
the
participant-menu-button-click
branch
from
d30fe41
to
283aec5
Compare
| /** | ||
| * Participant context menu button keys. | ||
| */ | ||
| export const PARTICIPANT_MENU_BUTTONS = { |
There was a problem hiding this comment.
style nit: sort variables alphabetically
There was a problem hiding this comment.
I mean PARTICIPANT_MENU_BUTTONS relative to VOLUME_SLIDER_SCALE
#13446
participantMenuButtonClickshould now listen to the buttons defined here https://github.com/jitsi/jitsi-meet/blob/6e816cafc4f1537f0e0d1f55f3f1aa97d666d71e/react/features/video-menu/constants.ts#L19 on top of the custom onesparticipantMenuButtonsWithNotifyClickconfig should work as its toolbar counterpart, enabling click prevention