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-57953] Host controls: Add more host notices #731

Merged
merged 15 commits into from May 14, 2024
Merged

[MM-57953] Host controls: Add more host notices #731

merged 15 commits into from May 14, 2024

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented May 10, 2024

Summary
image image

Ticket Link

@cpoile cpoile added 1: UX Review Requires review by ux 2: Dev Review Requires review by a core committer labels May 10, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

❗ No coverage uploaded for pull request base (MM-57470-host-mute-all@f6934fc). Click here to learn what that means.

Files Patch % Lines
server/host_controls.go 0.00% 4 Missing ⚠️
server/session.go 0.00% 4 Missing ⚠️
Additional details and impacted files 
@@                    Coverage Diff                    @@
##             MM-57470-host-mute-all     #731   +/-   ##
=========================================================
  Coverage                          ?   16.65%           
=========================================================
  Files                             ?       39           
  Lines                             ?     6696           
  Branches                          ?        0           
=========================================================
  Hits                              ?     1115           
  Misses                            ?     5457           
  Partials                          ?      124

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cpoile cpoile mentioned this pull request May 10, 2024
Merged
4 tasks
Base automatically changed from MM-57470-host-mute-all to main May 10, 2024 20:05
matthewbirtch
matthewbirtch reviewed May 10, 2024
View reviewed changes
webapp/src/components/host_notices.tsx Outdated

${({$onWidget}) => $onWidget && css`
width: 100%;
border-radius: 6px;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we adjust to 8px radius to match the main widget radius?

@cpoile cpoile requested a review from matthewbirtch May 10, 2024 20:55
matthewbirtch
matthewbirtch requested changes May 10, 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.

  1. Just a minor tweak on the corner radius (8px instead of 6px) for the notices that appear above the widget.

  2. The host icon seems scaled down in comparison to the others.
    image

  3. As a follow-up I think we'll probably want to adjust other notices to match style too. This can be done as a separate PR though if you'd rather

    image Screenshot 2024-05-10 at 4 44 01 PM

    I've added these in Figma here, but let me know if there are others beyond these

@cpoile
Copy link
Member Author

cpoile commented May 13, 2024

@matthewbirtch

  1. That's the same as the comment you made earlier, right? If so, I fixed in the previous commit.
  2. Sure thing, here you go:
image
  1. Could you make a ticket for those changes? And could you list the changes please? That will prevent some back and forth because it's tough to tell what changes were made by eyeballing (for example, I didn't realize that the border radius had been changed from what we had previously). Thanks!

@cpoile cpoile requested a review from matthewbirtch May 13, 2024 15:55
matthewbirtch
matthewbirtch approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@matthewbirtch matthewbirtch left a comment •
edited

Choose a reason for hiding this comment

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

Looks good. Thanks for the updates @cpoile. I've created a follow-up ticket for the other notices: https://mattermost.atlassian.net/browse/MM-58221

@streamer45 streamer45 removed the 1: UX Review Requires review by ux label May 14, 2024
@streamer45
Copy link
Contributor

@cpoile Two things:

  1. If a host has been assigned through the slash command in case they drop and rejoin they'd be host again but I don't see the notice showing. Is that expected?
  2. If the new host is the current user shouldn't we show "you are the host" to be consistent?

@cpoile
Copy link
Member Author

cpoile commented May 14, 2024

  1. No, good catch. Fixed.
  2. Good idea, here's what it looks like. I think there shouldn't be a bold around You, what do you think @matthewbirtch?
image

cpoile
cpoile commented May 14, 2024
View reviewed changes
webapp/src/websocket_handlers.ts
@@ -388,8 +388,8 @@ export function handleCallHostChanged(store: Store, ev: WebSocketMessage<CallHos
},
});

const profiles = profilesInCurrentCallMap(store.getState());
const hostProfile = profiles[ev.data.hostID];
const hostProfile = profilesInCurrentCallMap(store.getState())[ev.data.hostID] ||
Copy link
Member Author

Choose a reason for hiding this comment

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

This happens because we send out the host change before the join msg. I did this here bc it's less complicated than delaying the host change message server side.

streamer45
streamer45 approved these changes May 14, 2024
View reviewed changes
Copy link
Contributor

@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.

Thanks @cpoile , looks great!

@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 May 14, 2024
@streamer45 streamer45 added this to the v0.27.0 / MM 9.9 milestone May 14, 2024
@cpoile
Copy link
Member Author

cpoile commented May 14, 2024

Merging for now since we need to get master onto community -- if we need to bold the You then I can do that in one of the UI PRs.

@cpoile cpoile merged commit 716fd1d into main May 14, 2024
19 checks passed
@cpoile cpoile deleted the MM-57953-host-notices branch May 14, 2024 20:30
@matthewbirtch
Copy link
Contributor

Merging for now since we need to get master onto community -- if we need to bold the You then I can do that in one of the UI PRs.

Apologies for the delay. I agree with bolding the You in this notice @cpoile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@streamer45 streamer45 streamer45 approved these changes

@matthewbirtch matthewbirtch matthewbirtch 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.

None yet
4 participants
@cpoile @codecov-commenter @streamer45 @matthewbirtch