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-57964] General UX improvements #779

Merged
merged 17 commits into from
Jun 27, 2024
Merged

[MM-57964] General UX improvements #779

merged 17 commits into from
Jun 27, 2024

Conversation

streamer45
Copy link
Collaborator

Summary

PR makes the following usability improvements:

  • (popout) Reordered controls to match widget.
  • (popout) Moved the participant's count next to the icon.
  • Implemented start/stop recording menu button on widget.
  • Implemented show call thread functionality on widget (requires desktop changes).
  • Screen permissions state should now be synched 2-way across widget and popout.

Designs

https://www.figma.com/design/1Pw8BuAVglnKOlQqmDrOUC/MM-57964-Consistent-controls-in-widget-and-pop-out-window?node-id=1-7&m=dev

Ticket Link

https://mattermost.atlassian.net/browse/MM-57964

@streamer45 streamer45 added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels Jun 14, 2024
@streamer45 streamer45 added this to the v0.29.0 / MM 9.11 milestone Jun 14, 2024
@streamer45 streamer45 self-assigned this Jun 14, 2024
@@ -51,7 +51,7 @@
"@babel/preset-typescript": "7.16.0",
"@formatjs/cli": "5.0.7",
"@mattermost/client": "file:mattermost-webapp/webapp/platform/client",
"@mattermost/desktop-api": "5.7.0-3",
"@mattermost/desktop-api": "https://gitpkg.now.sh/mattermost/desktop/api-types?MM-57964",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just temporary to get it to pass.

@streamer45 streamer45 added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Jun 14, 2024
@streamer45
Copy link
Collaborator Author

Mmm, I realized that showing the stop recording modal on Desktop (from widget) will require some more work on both sides.

@streamer45 streamer45 added the Work In Progress Not yet ready for review label Jun 14, 2024
@streamer45 streamer45 removed the Work In Progress Not yet ready for review label Jun 17, 2024
@streamer45
Copy link
Collaborator Author

Should be good for a first pass now.

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Nice! Looks great in action too.

export const selectRHSPost = (postID: string): ActionFuncAsync => {
return async (dispatch: DispatchFunc) => {
if (window.ProductApi) {
dispatch(window.ProductApi.selectRhsPost(postID));
Copy link
Member

Choose a reason for hiding this comment

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

Is this guaranteed to be on ProductApi? If not, we should optionally call it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose nothing is guaranteed when it comes to webapp :p But it's been there forever if that's what you are asking.

logDebug('desktopAPI.openStopRecordingModal');
window.desktopAPI.openStopRecordingModal(this.props.channel.id);
} else {
this.props.stopCallRecording(this.props.channel.id);
Copy link
Member

Choose a reason for hiding this comment

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

I guess we don't have a confirmation for desktop pre-openStopRecordingModal?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I still wanted to give some functionality rather than hide the button. Happy to revisit though.

webapp/src/components/call_widget/component.tsx Outdated Show resolved Hide resolved
webapp/src/components/call_widget/component.tsx Outdated Show resolved Hide resolved
@@ -469,6 +476,28 @@ export default class Plugin {
}));
}

if (window.desktopAPI?.onOpenThread) {
logDebug('registering desktopAPI.onOpenThread');
this.unsubscribers.push(window.desktopAPI.onOpenThread((threadID: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

This desktopAPI.on... is slick, nice work.

@streamer45 streamer45 removed the 2: Dev Review Requires review by a core committer label Jun 19, 2024
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 @streamer45! Just a couple of small requests:

  1. When you click on Record call from the widget, while it takes a few seconds for the recording to actually begin, the menu item stays as Record call and clicking on it again triggers an error (even though the recording starts after that). Ideally, it should immediately change to Stop recording even if the recording has not really started. This is how the record button in the pop out window works.
Widget.mov
  1. In the widget, for the Stop recording menu item, can we also change the color of the text to the same red as the icon? They shouldn't have different colors.
image

Also, I didn't see the Show chat thread menu item on the widget in the desktop app, but I'm assuming that'll be done separately. Do let me know if you need me to review this again. Thanks!

@streamer45
Copy link
Collaborator Author

Thanks @streamer45! Just a couple of small requests:

  1. When you click on Record call from the widget, while it takes a few seconds for the recording to actually begin, the menu item stays as Record call and clicking on it again triggers an error (even though the recording starts after that). Ideally, it should immediately change to Stop recording even if the recording has not really started. This is how the record button in the pop out window works.

Widget.mov
2. In the widget, for the Stop recording menu item, can we also change the color of the text to the same red as the icon? They shouldn't have different colors.

image

Updated to match the rest of UI.

Also, I didn't see the Show chat thread menu item on the widget in the desktop app, but I'm assuming that'll be done separately. Do let me know if you need me to review this again. Thanks!

@abhijit-singh It's implemented, but since it required Desktop changes you'd need to run the app from master branch. We don't render the menu item if the new functionality to open a thread is not detected since it wouldn't work.

@streamer45
Copy link
Collaborator Author

@abhijit-singh Let me know if this is good to go from a UX perspective.

@abhijit-singh
Copy link

Yep, good to go from my side @streamer45

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 1: UX Review Requires review by ux Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Jun 25, 2024
@streamer45 streamer45 merged commit ea93eb2 into main Jun 27, 2024
18 checks passed
@streamer45 streamer45 deleted the MM-57964 branch June 27, 2024 07:48
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