-
Notifications
You must be signed in to change notification settings - Fork 43
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-52857 - DM/GM ringing #445
Conversation
# Conflicts: # webapp/src/actions.ts # webapp/src/index.tsx # webapp/src/reducers.ts
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #445 +/- ##
========================================
+ Coverage 5.65% 5.67% +0.01%
========================================
Files 22 22
Lines 4104 4162 +58
========================================
+ Hits 232 236 +4
- Misses 3856 3909 +53
- Partials 16 17 +1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Did a first pass and left mostly minor comments.
webapp/src/utils.ts
Outdated
@@ -77,6 +77,15 @@ export function getChannelURL(state: GlobalState, channel: Channel, teamId: stri | |||
return channelURL; | |||
} | |||
|
|||
export function shouldRenderCallsIncoming() { | |||
const win = window.opener ? window.opener : window; | |||
if (win.desktop && window.location.pathname.indexOf('/messages/') === -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if the /messages/
path is going away any time soon? Just want to make sure we prepare for it if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question.
I was kind of assuming /messages
wouldn't change... But it might... And anyway, I discovered this was wrong (you could break it under some combinations of reloading and where you were in the app). So I've changed the index.tsx
and the call_container
to fix that, and I changed this util fn to do the check a bit more safely. Hopefully this will be a bit more robust for when we remove the tabs: there's a good chance that when the tabs are removed we will need to display the LHS incoming widget no matter what you're looking at, and this should work then.
(Remember, the reason we're doing this check is that we don't want multiple LHS incoming containers to ring at the same time, which is what was happening because the desktop loaded separate webapps per tab.)
# Conflicts: # webapp/i18n/en.json # webapp/package-lock.json # webapp/package.json # webapp/src/utils.ts
@streamer45 Fixed the remaining issues, I think we're good for another pass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A quick first pass tonight and here are a few things I'm noticing:
-
When the team sidebar is hidden, the width of the banners is more narriw and it looks like the join/ignore buttons are not respecting the padding and the join button is even extending beyond the container somehow. One way we could combat this is to reduce the horizontal padding on these buttons to 16px instead of 20px. We could also reduce the space between buttons to 8px instead of 12px.
-
The onboarding checklist floating button overlaps the new DM ringing banner. I think we can place the ringing banner at a higher z-index
-
In the new ringing banner, can we reduce the
margin-top
on the buttons row from 16px to 12px?
-
For the hover state on the
Ignore
button, let's go withbackground: rgba(var(--button-color-rgb),0.16);
-
If I join the DM call by clicking the top canner in the DM channel or the call card in the message area, the new banner shows as if another call is still ringing, but I'm in the call already
Screen.Recording.2023-06-14.at.9.12.22.PM.mov
I still need to test the following:
- scenarios of other multiple DM calls at once
- GMs
- popup window
- desktop app
Thanks @matthewbirtch for the great comments -- I've made the adjustments, much much better now. The z-index works (those are both above the onboarding tour button). @streamer45 In the latest PR I migrated the ChannelCallToast component so that I could reuse the |
I'm not sure what's happening with the channel toast e2e diff -- it passes locally, and I can't see why it would be narrower in the trace. |
The e2e pipeline is currently half broken. It doesn't even start the tests. See https://community.mattermost.com/private-core/pl/7qm1ndx8hjy9fqm7mtxbp5smkc |
webapp/src/actions.ts
Outdated
const otherUser = getUser(getState(), hostID); | ||
if (!otherUser) { | ||
await dispatch(getProfilesByIdsAction([hostID])); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this apply to GM? Maybe it's just a naming thing but it looks like what we really need here is the caller, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I made a wrong assumption? I was assuming the hostID was the person who started the call (i.e., the caller).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just the name that's confusing. Calling it otherUser
when we mean the caller. In the context of a DM it's fine I suppose but in a GM it feels weird. No problem with the logic, although technically the host could change so it's not always an exact mapping. What you really want would be what we set as OwnerID
, not sure if we expose it though but not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree -- I fixed it by moving over to ownerID and then referring to them as the Caller from then on. in ade6649
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Conflicts: # webapp/src/index.tsx
Thanks @matthewbirtch, I made a ticket for the tooltip: https://mattermost.atlassian.net/browse/MM-53320 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cpoile , amazing effort 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Chris! I think this looks good to me!
* refactor & add hooks; notify when in background (desktop & webapp) * desktop notifications, ringing rules, simplification of ringing * finish rebase, mostly channelID -> callID changes * use main branch calls-common; enable ringingEnabled feature flag * small bug from switching to callID * couple more small fixes * respect user and channel muted settings * more granular notification preferences (channel + system level) * ringing should default to on if not set * use ownerID as caller, hostID -> callerID; fix notification bug * small fix * i18n * MM-53322 - Channel toast fixes (#455) * record explicit dismisses for channel toast and for later use * e2e tests * Translated using Weblate (Japanese) (#457) Currently translated at 100.0% (161 of 161 strings) Merge remote-tracking branch 'origin/main' Translated using Weblate (Korean) Currently translated at 3.1% (5 of 161 strings) Translated using Weblate (Czech) Currently translated at 99.3% (160 of 161 strings) Translated using Weblate (French) Currently translated at 60.2% (97 of 161 strings) Merge remote-tracking branch 'origin/main' Merge remote-tracking branch 'origin/main' Translated using Weblate (Polish) Currently translated at 100.0% (161 of 161 strings) Translated using Weblate (Dutch) Currently translated at 100.0% (161 of 161 strings) Translated using Weblate (Spanish) Currently translated at 30.4% (49 of 161 strings) Translated using Weblate (German) Currently translated at 100.0% (161 of 161 strings) Update translation files Updated by "Cleanup translation files" hook in Weblate. Translated using Weblate (Spanish) Currently translated at 26.7% (43 of 161 strings) Translated using Weblate (Czech) Currently translated at 100.0% (161 of 161 strings) Translated using Weblate (Chinese (Simplified)) Currently translated at 100.0% (161 of 161 strings) Merge remote-tracking branch 'origin/main' Translated using Weblate (German) Currently translated at 100.0% (161 of 161 strings) Translated using Weblate (Italian) Currently translated at 26.0% (42 of 161 strings) Translated using Weblate (Dutch) Currently translated at 100.0% (161 of 161 strings) Translated using Weblate (Korean) Currently translated at 1.8% (3 of 161 strings) Translated using Weblate (Croatian) Currently translated at 96.8% (156 of 161 strings) Translated using Weblate (Polish) Currently translated at 100.0% (161 of 161 strings) Added translation using Weblate (Korean) Translated using Weblate (Turkish) Currently translated at 100.0% (161 of 161 strings) Translated using Weblate (Chinese (Simplified)) Currently translated at 63.9% (103 of 161 strings) Translated using Weblate (Japanese) Currently translated at 100.0% (161 of 161 strings) Merge remote-tracking branch 'origin/main' Translated using Weblate (Japanese) Currently translated at 100.0% (159 of 159 strings) Translated using Weblate (Turkish) Currently translated at 100.0% (159 of 159 strings) Translated using Weblate (Japanese) Currently translated at 1.8% (3 of 159 strings) Translated using Weblate (Croatian) Currently translated at 95.5% (152 of 159 strings) Translated using Weblate (Chinese (Simplified)) Currently translated at 15.7% (25 of 159 strings) Translated using Weblate (Japanese) Currently translated at 1.2% (2 of 159 strings) Translated using Weblate (Uzbek) Currently translated at 4.4% (7 of 159 strings) Added translation using Weblate (Uzbek) Translated using Weblate (Italian) Currently translated at 20.1% (32 of 159 strings) Added translation using Weblate (Italian) Translated using Weblate (Czech) Currently translated at 100.0% (159 of 159 strings) Translated using Weblate (Croatian) Currently translated at 94.9% (151 of 159 strings) Translated using Weblate (Croatian) Currently translated at 1.2% (2 of 159 strings) Added translation using Weblate (Croatian) Translated using Weblate (German) Currently translated at 100.0% (159 of 159 strings) Translated using Weblate (Dutch) Currently translated at 100.0% (159 of 159 strings) Translated using Weblate (Polish) Currently translated at 100.0% (159 of 159 strings) Translate-URL: https://translate.mattermost.com/projects/calls/webapp/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/cs/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/de/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/es/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/fr/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/hr/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/it/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/ja/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/ko/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/nl/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/pl/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/tr/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/uz/ Translate-URL: https://translate.mattermost.com/projects/calls/webapp/zh_Hans/ Translation: Calls/webapp Co-authored-by: Alberto Lozano <alferto82@gmail.com> Co-authored-by: GJ\" Guenjun Yoo <guenjun.yoo@outlook.com> Co-authored-by: Gianluigi Fiorillo <ggflower@gmail.com> Co-authored-by: Jihyeon Gim <potatogim@potatogim.net> Co-authored-by: Kaya Zeren <kayazeren@gmail.com> Co-authored-by: Kwangoh Moon <raymond.moon@gmail.com> Co-authored-by: Michele Amato <mike.amato85@tiscali.it> Co-authored-by: Milo Ivir <mail@milotype.de> Co-authored-by: Nathanaël <contact@nathanaelhoun.fr> Co-authored-by: Pan Klobouk <julda.fulda@centrum.cz> Co-authored-by: Sarvarbek Hasanov <admin@softex.uz> Co-authored-by: Tom De Moor <tom@controlaltdieliet.be> Co-authored-by: jprusch <rs@schaeferbarthold.de> Co-authored-by: kaakaa <stooner.hoe@gmail.com> Co-authored-by: master7 <marcin.karkosz@rajska.info> Co-authored-by: ondraknezour <knezour@weboutsourcing.cz> Co-authored-by: orta-contrib <orta.contrib@gmail.com> Co-authored-by: tianyazi <1278881217@qq.com> Co-authored-by: timmycheng <timmycheng@foxmail.com> * Translated using Weblate (Korean) (#456) Currently translated at 100.0% (2 of 2 strings) Translated using Weblate (Spanish) Currently translated at 100.0% (2 of 2 strings) Translated using Weblate (Chinese (Simplified)) Currently translated at 100.0% (2 of 2 strings) Merge remote-tracking branch 'origin/main' Added translation using Weblate (Korean) Merge remote-tracking branch 'origin/main' Merge remote-tracking branch 'origin/main' Merge remote-tracking branch 'origin/main' Translated using Weblate (Japanese) Currently translated at 100.0% (2 of 2 strings) Translated using Weblate (Japanese) Currently translated at 50.0% (1 of 2 strings) Translated using Weblate (Chinese (Simplified)) Currently translated at 100.0% (2 of 2 strings) Added translation using Weblate (Uzbek) Translated using Weblate (Italian) Currently translated at 100.0% (2 of 2 strings) Added translation using Weblate (Italian) Translated using Weblate (Croatian) Currently translated at 100.0% (2 of 2 strings) Added translation using Weblate (Croatian) Translate-URL: https://translate.mattermost.com/projects/calls/standalone/es/ Translate-URL: https://translate.mattermost.com/projects/calls/standalone/hr/ Translate-URL: https://translate.mattermost.com/projects/calls/standalone/it/ Translate-URL: https://translate.mattermost.com/projects/calls/standalone/ja/ Translate-URL: https://translate.mattermost.com/projects/calls/standalone/ko/ Translate-URL: https://translate.mattermost.com/projects/calls/standalone/zh_Hans/ Translation: Calls/standalone Co-authored-by: Alberto Lozano <alferto82@gmail.com> Co-authored-by: GJ\" Guenjun Yoo <guenjun.yoo@outlook.com> Co-authored-by: Gianluigi Fiorillo <ggflower@gmail.com> Co-authored-by: Milo Ivir <mail@milotype.de> Co-authored-by: kaakaa <stooner.hoe@gmail.com> Co-authored-by: orta-contrib <orta.contrib@gmail.com> Co-authored-by: tianyazi <1278881217@qq.com> Co-authored-by: timmycheng <timmycheng@foxmail.com> * MM-53261 - Expanded view notifications (#461) * incoming calls on expanded window; simplify global join * stack the notifications vertically * dismiss notification from SwitchCallModal; some formatting --------- Co-authored-by: Weblate (bot) <hosted@weblate.org> Co-authored-by: Alberto Lozano <alferto82@gmail.com> Co-authored-by: GJ\" Guenjun Yoo <guenjun.yoo@outlook.com> Co-authored-by: Gianluigi Fiorillo <ggflower@gmail.com> Co-authored-by: Jihyeon Gim <potatogim@potatogim.net> Co-authored-by: Kaya Zeren <kayazeren@gmail.com> Co-authored-by: Kwangoh Moon <raymond.moon@gmail.com> Co-authored-by: Michele Amato <mike.amato85@tiscali.it> Co-authored-by: Milo Ivir <mail@milotype.de> Co-authored-by: Nathanaël <contact@nathanaelhoun.fr> Co-authored-by: Pan Klobouk <julda.fulda@centrum.cz> Co-authored-by: Sarvarbek Hasanov <admin@softex.uz> Co-authored-by: Tom De Moor <tom@controlaltdieliet.be> Co-authored-by: jprusch <rs@schaeferbarthold.de> Co-authored-by: kaakaa <stooner.hoe@gmail.com> Co-authored-by: master7 <marcin.karkosz@rajska.info> Co-authored-by: ondraknezour <knezour@weboutsourcing.cz> Co-authored-by: orta-contrib <orta.contrib@gmail.com> Co-authored-by: tianyazi <1278881217@qq.com> Co-authored-by: timmycheng <timmycheng@foxmail.com> --------- Co-authored-by: Weblate (bot) <hosted@weblate.org> Co-authored-by: Alberto Lozano <alferto82@gmail.com> Co-authored-by: GJ\" Guenjun Yoo <guenjun.yoo@outlook.com> Co-authored-by: Gianluigi Fiorillo <ggflower@gmail.com> Co-authored-by: Jihyeon Gim <potatogim@potatogim.net> Co-authored-by: Kaya Zeren <kayazeren@gmail.com> Co-authored-by: Kwangoh Moon <raymond.moon@gmail.com> Co-authored-by: Michele Amato <mike.amato85@tiscali.it> Co-authored-by: Milo Ivir <mail@milotype.de> Co-authored-by: Nathanaël <contact@nathanaelhoun.fr> Co-authored-by: Pan Klobouk <julda.fulda@centrum.cz> Co-authored-by: Sarvarbek Hasanov <admin@softex.uz> Co-authored-by: Tom De Moor <tom@controlaltdieliet.be> Co-authored-by: jprusch <rs@schaeferbarthold.de> Co-authored-by: kaakaa <stooner.hoe@gmail.com> Co-authored-by: master7 <marcin.karkosz@rajska.info> Co-authored-by: ondraknezour <knezour@weboutsourcing.cz> Co-authored-by: orta-contrib <orta.contrib@gmail.com> Co-authored-by: tianyazi <1278881217@qq.com> Co-authored-by: timmycheng <timmycheng@foxmail.com>
# Conflicts: # webapp/src/actions.ts
Summary
This is the first step in an epic of DM/GM ringing. Apologies for the single commit, it's all kind of related. For now, the following has been completed (also useful for a testing plan):
TODO: (I will make tickets for these)
To try this out, you'll need: mattermost/mattermost#23444
And you'll need to tag this commit with v0.17.0 before deploying.
Some screenshots
I think I need to get some UX input on hover states, I'll leave that for a polish stage -- this is mostly for functionality.
Ticket Link
Fixes #125
Fixes #427