Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[MM-57464] Host controls: Lower hand; host controls notifications #705

Merged
merged 16 commits into from
Apr 30, 2024
Merged

[MM-57464] Host controls: Lower hand; host controls notifications #705

merged 16 commits into from
Apr 30, 2024

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented Apr 19, 2024

Summary

  • This one is a bit more complicated because I'm adding a host controls notification to the expanded view and the widget (widget wasn't in the designs but I figured it would be wanted). I chose it to stay for 5s (changeable if you want), and added animation for the expanded view.
  • I'm getting the display name in the handler so that we avoid the problem of a host doing something and then leaving the call and their name becoming "someone".
  • They stack (as you see from me playing around in the screenshots), for that edge-case. The host control message takes precedence over others, since it seems time-sensitive and more important.
the menu command fooling around with stacking raising hand again after it being lowered
image image image

Ticket Link

@cpoile cpoile changed the title [MM-57464] Host controls: Unraise hand; host controls notifications [MM-57464] Host controls: Lower hand; host controls notifications Apr 19, 2024
@cpoile cpoile requested a review from streamer45 April 23, 2024 21:48
@cpoile cpoile added the 2: Dev Review Requires review by a core committer label Apr 23, 2024
@cpoile cpoile added the 1: UX Review Requires review by ux label Apr 23, 2024
matthewbirtch
matthewbirtch reviewed Apr 24, 2024
View reviewed changes
Copy link
Contributor

@matthewbirtch matthewbirtch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome @cpoile. Nice work. The only thing I noticed is that in the popout window, the 'user raised hand' notice jumps around strangely when it gets cleared and the 'host lowered your hand' notice animates in (see video below). This seems to happen in both the desktop app and browser. Can we make that cleaner?

Screen.Recording.2024-04-24.at.10.24.04.AM.mov

@cpoile
Copy link
Member Author

cpoile commented Apr 25, 2024

Thanks @matthewbirtch , I've fixed it, let me know if this works for you. I can delay the "host lowered your hand" chip even more if you want there to be a moment before it shows.

matthewbirtch
matthewbirtch approved these changes Apr 25, 2024
View reviewed changes
Copy link
Contributor

@matthewbirtch matthewbirtch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me @cpoile!

@matthewbirtch matthewbirtch removed the 1: UX Review Requires review by ux label Apr 25, 2024
streamer45
streamer45 reviewed Apr 25, 2024
View reviewed changes
webapp/src/selectors.ts Show resolved Hide resolved
server/websocket.go Outdated Show resolved Hide resolved
webapp/src/components/host_notifications.tsx Show resolved Hide resolved
webapp/src/websocket_handlers.ts Show resolved Hide resolved
@streamer45 streamer45 added this to the v0.27.0 / MM 9.9 milestone Apr 26, 2024
@cpoile cpoile requested a review from streamer45 April 26, 2024 18:39
streamer45
streamer45 reviewed Apr 26, 2024
View reviewed changes
server/api_router.go Outdated
@@ -98,6 +98,7 @@ func (p *Plugin) newAPIRouter() *mux.Router {
hostCtrlRouter.HandleFunc("/make", p.handleMakeHost).Methods("POST")
hostCtrlRouter.HandleFunc("/mute", p.handleMuteSession).Methods("POST")
hostCtrlRouter.HandleFunc("/screen-off", p.handleScreenOff).Methods("POST")
hostCtrlRouter.HandleFunc("/unraise-hand", p.handleUnraiseHand).Methods("POST")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also expect this to become lower-hand, and in the new methods as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦

webapp/src/components/expanded_view/component.tsx
@@ -1350,6 +1350,7 @@ const ReactionOverlay = styled.div`
display: flex;
flex-direction: column;
gap: 12px;
pointer-events: none;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is what you mentioned before. Will events pass through without altering the stack order?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, which is what we want -- we don't want to mess with the z-index of the reactions or notifications or error message boxes, those have to be on top, but we don't want the div they're on to intercept the pointer events.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will need to merge this into my work as well, thanks.

@cpoile cpoile requested a review from streamer45 April 26, 2024 19:11
streamer45
streamer45 approved these changes Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@streamer45 streamer45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

webapp/src/actions.ts Outdated
@@ -596,3 +596,13 @@ export const stopScreenshare = async (callID: string, sessionID: string) => {
},
);
};

export const unraiseHand = async (callID: string, sessionID: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> lowerHand, you asked for it :)

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Apr 26, 2024
@cpoile cpoile merged commit e7ed885 into main Apr 30, 2024
19 checks passed
@cpoile cpoile deleted the MM-57464-lower-hand branch April 30, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbirtch matthewbirtch matthewbirtch approved these changes

@streamer45 streamer45 streamer45 approved these changes

Assignees
No one assigned
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Milestone
v0.27.0 / MM 9.9
Development

Successfully merging this pull request may close these issues.
3 participants
@cpoile @streamer45 @matthewbirtch