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.

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

[MM-58284] Confirmation when leaving a call #763

Merged
merged 10 commits into from
Jun 12, 2024

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented Jun 4, 2024

Summary

  • Added confirmations on the widget, global widget, expanded view, and post component's leave call buttons
  • The global widget required an added shim to give room for the confirmation, please see if it works well on linux desktop :)
  • The popup system needed a bit of reworking to add the tooltips, kind of a mess, but I think I figured out how to do it in an elegant way that doesn't affect the existing uses (host controls), but would appreciate a second pair of eyes on that just in case (that the tooltips look correct in all places on the leave button, and that the host controls still work/look as expected).

First row is from desktop and host, second row is from participant and webapp:

image image image
image image image

Ticket Link

@cpoile cpoile added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Jun 4, 2024
@cpoile cpoile added this to the v0.28.0 / MM 9.10 milestone Jun 4, 2024
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.

Looks great, even on 🐧 :)

Two things I noticed:

  • The keyboard shortcut causes the user to leave immediately. This is fine but inconsistent with how we are handling a similar case in [MM-57365] Stop recording confirmation modal #760.
  • Ending the call is not working as I'd expect, although this is unrelated to this PR. The call post should update immediately but for some reason, that's not happening until the server timeout triggers. We should look into this.

e2e/.eslintrc.json Show resolved Hide resolved
webapp/src/components/expanded_view/component.tsx Outdated Show resolved Hide resolved
@cpoile
Copy link
Member Author

cpoile commented Jun 6, 2024

@streamer45 for 1: Earlier I had messaged @abhijit-singh about it too. I suggested we keep the shortcut as it has been, under the assumption that if you hit it that means you want it to happen right then, not that you then want to use the mouse and click again. Your right that it doesn't match the end recording, but in that case the action is more destructive, whereas you can always rejoin if you left by accident. Imo, it just seems annoying that the shortcut just opens a little popup. If we made it do that, I would vote we just remove the shortcut altogether for uselessness. :)

For 2, I can look into that. I think it's been happening like that for a long time.

@cpoile cpoile closed this Jun 6, 2024
@cpoile cpoile reopened this Jun 6, 2024
@streamer45
Copy link
Collaborator

@streamer45 for 1: Earlier I had messaged @abhijit-singh about it too. I suggested we keep the shortcut as it has been, under the assumption that if you hit it that means you want it to happen right then, not that you then want to use the mouse and click again. Your right that it doesn't match the end recording, but in that case the action is more destructive, whereas you can always rejoin if you left by accident. Imo, it just seems annoying that the shortcut just opens a little popup. If we made it do that, I would vote we just remove the shortcut altogether for uselessness. :)

Yeah, I agree. Shortcut to show a popup is weird.

For 2, I can look into that. I think it's been happening like that for a long time.

Thanks.

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.

Great! And thanks for the brave effort on e2e and related lint checks.

@streamer45 streamer45 removed the 2: Dev Review Requires review by a core committer label Jun 6, 2024
@cpoile
Copy link
Member Author

cpoile commented Jun 6, 2024

@streamer45 Fixed how we were doing call end, and exposing the end call for everyone button to admins.

@cpoile cpoile requested a review from streamer45 June 6, 2024 15:12
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.

Working great now!

@cpoile
Copy link
Member Author

cpoile commented Jun 6, 2024

fyi: added a rule to only show End call for everyone when there are other people in the call.

Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Thanks @cpoile! Looks good! I have a few requests -

  1. In the expanded window, can we align the right edge of the confirmation pop-out with the right edge of the button?
  2. Also in the expanded window, can we make the border radius for the pop-out 8px to match that of the button?

Current:
image

Ideal:
image

Note that the above two requests apply to the popout window only.
3. Can we increase the width of the menu items to 36px to be consistent with the rest of the app? Right now it seems to be 32px.

Current:
image

  1. On the Desktop App, in the floating widget, clicking outside the popout menu (within the widget boundries, does not close the popout menu. Ideally it should close when clicked outside.

@cpoile
Copy link
Member Author

cpoile commented Jun 11, 2024

@abhijit-singh 1-3 done, thanks! 4 took me awhile, but I don't think we can solve it. The problem is that when you click on another part of the widget that is meant to be draggable, the click doesn't register (but you can drag). If you click on any place you can click, the menu will close (e.g. on a participant in the participant list, or any of the widget buttons). So unfortunately I think we're stuck with that -- the user will have to click on the Cancel button if they change their mind (or click on a participant or settings menu item).

Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @cpoile!

@cpoile cpoile added 3: Reviews Complete All reviewers have approved the pull request and removed 1: UX Review Requires review by ux labels Jun 12, 2024
@cpoile cpoile merged commit aa849d5 into main Jun 12, 2024
20 checks passed
@cpoile cpoile deleted the MM-58284-end-call-confirmation branch June 12, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants