Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-23120 Custom status issues #7843

Merged
merged 11 commits into from
Apr 20, 2021
Merged

MM-23120 Custom status issues #7843

merged 11 commits into from
Apr 20, 2021

Conversation

manojmalik20
Copy link
Contributor

@manojmalik20 manojmalik20 commented Apr 8, 2021 •
edited
Loading

Summary

  • Fixed MM-34095 Show recent statuses on the set status confirmation screen for more efficient status switching
  • Fixed MM-33149 Updated "Mobile Push Notifications" text in Account Settings
  • Fixed MM-33195 Custom Status emoji misaligned in profile popover
  • Fixed MM-33246 Custom Status Modal: increase max height so that all default options are visible without scrolling
  • Fixed MM-33433 Custom statuses should appear in channel switcher
  • Fixed MM-34571 Tooltip for custom status without text has emoji misaligned
  • Fixed MM-33431 Custom status emoji size match the emoji size used in reactions and alignment needs tweaking
  • Fixed MM-33214 With RHS open, pin icon and channel description overlap statuses in lower browser widths
  • Fixed MM-33220 Reduce opacity of custom status emoji in LHS DMs

Ticket Link

Related Pull Requests

#7750

@manojmalik20
Fixed the MM-34095 issue
1626f28
@manojmalik20
Solved several issues from the MM-23120 epic
0dd5bb0 
Solved the MM-33149 issue
Solved the MM-33195 issue
Solved the MM-33214 issue
Solved the MM-33220 issue
Solved the MM-33246 issue
Solved the MM-33433 issue
Solved the MM-34571 issue
Solved the MM-33431 issue
@mattermod
Copy link
Contributor

Hello @manojosh,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@mattermod mattermod requested a review from Willyfrog April 8, 2021 14:26
@mattermod mattermod added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Apr 8, 2021
@esethna esethna added the 1: UX Review Requires review by a UX Designer label Apr 8, 2021
@esethna esethna added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Apr 8, 2021
@esethna esethna added this to the v5.35 milestone Apr 8, 2021
matthewbirtch
matthewbirtch suggested changes Apr 8, 2021
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.

Thanks @manojosh! Nice work resolving these issues. I just have a few really minor points:

  1. In the Channel Switcher, can we make the left margin on the custom status 8px instead of 4px? It feels like it needs a bit more space there
  2. This may not be related to this PR, but the bullet after the status in the DM channel header feels odd now. I wonder if we should just consider removing this since the spacing may provide enough differentiation here

Screen Shot 2021-04-08 at 11 29 20 AM

3. For the reduced opacity emojis in the left sidebar, can we make them full opacity when you hover over them?

@esethna
Copy link
Contributor

esethna commented Apr 8, 2021

Thanks @manojosh, one thing I noticed: The modal height should dynamically resize to accommodate all selections as noted in the ticket (note that the max options showing would be 10: 5 recent and 5 suggestions), right now it is scrolling to reveal all options: https://mattermost.atlassian.net/browse/MM-33246
image

@manojmalik20
Fixed some UI issues
431ada1 
Removed the dot in channel header after custom status
Changed the marginLeft for the custom status emoji in switch channel modal
Changed opacity of custom status emoji on hover
@manojmalik20
Copy link
Contributor Author

@matthewbirtch All the changes are done.

@esethna If we don't want any scrolling to accommodate all the 10 suggestions then the modal looks like this.

Screenshot from 2021-04-09 13-36-32

So we have to change the positioning of the modal. Right now, it's done by giving margin-top to the modal as calc(50vh - 240px)
I thought of changing it to calc(50vh - 320px). Here is how it looks

Screenshot from 2021-04-09 13-47-09
Screenshot from 2021-04-09 13-47-27

Is this fine or do you want it to be dynamically changed as the no. of recent statuses increase? If so, what is the minimum value of margin-top which I should use?

@matthewbirtch
Copy link
Contributor

@matthewbirtch All the changes are done.

@esethna If we don't want any scrolling to accommodate all the 10 suggestions then the modal looks like this.

Screenshot from 2021-04-09 13-36-32

So we have to change the positioning of the modal. Right now, it's done by giving margin-top to the modal as calc(50vh - 240px)
I thought of changing it to calc(50vh - 320px). Here is how it looks

Screenshot from 2021-04-09 13-47-09
Screenshot from 2021-04-09 13-47-27

Is this fine or do you want it to be dynamically changed as the no. of recent statuses increase? If so, what is the minimum value of margin-top which I should use?

@esethna I think we should consider this effort as a separate PR if we want to merge this in for the next release. Accommadating for 10 options without scrolling makes for a pretty tall modal and I don't think we should do this. Also, it wouldn't work well for people on smaller resolution. Can we leave it as-is for now and create a follow-up ticket to look in to dynamically resizing if we feel strongly?

@esethna
Copy link
Contributor

esethna commented Apr 9, 2021

@matthewbirtch @manojosh okay that's fine, we can leave as is. But I'm not clear on what changed in this PR from the existing behaviour? Can we just update the ticket to reflect what changed so QA can test appropriately?

@manojmalik20
Copy link
Contributor Author

@esethna Actually what changed was the min and max height of the modal. Earlier if any one of the suggestions was present in the recents then the Recents title would appear along with the scrollbar but now the height is adjusted to accommodate all the suggestions along with the Recents and Suggestions titles.

Refer to the gif here https://mattermost.atlassian.net/browse/MM-33246

@matthewbirtch
Copy link
Contributor

  1. In the Channel Switcher, can we make the left margin on the custom status 8px instead of 4px? It feels like it needs a bit more space there
  2. This may not be related to this PR, but the bullet after the status in the DM channel header feels odd now. I wonder if we should just consider removing this since the spacing may provide enough differentiation here
Screen Shot 2021-04-08 at 11 29 20 AM
  1. For the reduced opacity emojis in the left sidebar, can we make them full opacity when you hover over them?

These changes all look to be resolved now. I'll approve

@amyblais amyblais removed the 1: UX Review Requires review by a UX Designer label Apr 9, 2021
@esethna
Copy link
Contributor

esethna commented Apr 9, 2021

height is adjusted to accommodate all the suggestions along with the Recents and Suggestions titles.

Got it, I adjusted the ticket description, thanks

@esethna
Copy link
Contributor

esethna commented Apr 9, 2021

@michelengelen @Willyfrog for dev review, thanks!

michelengelen
michelengelen approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@michelengelen michelengelen left a comment

Choose a reason for hiding this comment

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

Only a small suggestion from my side ... otherwise LGTM! Thanks for your contribution! 🙇🏻‍♂️

components/custom_status/custom_status_modal.tsx Outdated Show resolved Hide resolved
Willyfrog
Willyfrog approved these changes Apr 12, 2021
View reviewed changes
Copy link
Contributor

@Willyfrog Willyfrog left a comment

Choose a reason for hiding this comment

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

LGTM

@amyblais amyblais removed the 2: Dev Review Requires review by a core commiter label Apr 12, 2021
@manojmalik20
Copy link
Contributor Author

No problem @michelengelen.. Resolving the conflicts

@michelengelen
Copy link
Contributor

Thanks a lot @manojmalik20 ... will merge it now!

@michelengelen michelengelen merged commit c2c4725 into mattermost:master Apr 20, 2021
@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@mattermod mattermod removed the AutoMerge used by Mattermod to merge PR automatically label Apr 20, 2021
@mattermod
Copy link
Contributor

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Failed to add the RSA host key for IP address '140.82.113.3' to the list of known hosts (/app/.ssh/known_hosts).
From github.com:mattermost/mattermost-webapp
   60d7119c5..383ce8b9b  MM-32962_dependency-updates -> upstream/MM-32962_dependency-updates
   92e045091..ba42ce9f4  MM-34758   -> upstream/MM-34758
 * [new branch]          MM-35021   -> upstream/MM-35021
   94428f45c..00baa5885  cloud      -> upstream/cloud
   9757eb1a6..c2c4725be  master     -> upstream/master
Fetching origin
Failed to add the RSA host key for IP address '140.82.113.3' to the list of known hosts (/app/.ssh/known_hosts).
+++ Updating remotes done...
!!! 'upstream/release-5.35' not found. The second argument should be something like upstream/release-0.21.
    (In particular, it needs to be a valid, existing remote branch that I can 'git checkout'.)

+++ Returning you to the master branch and cleaning up.

@Willyfrog
Copy link
Contributor

/cherry-pick cloud

@mattermod
Copy link
Contributor

Cherry pick is scheduled.

@mattermod
Copy link
Contributor

Error trying doing the automated Cherry picking. Please do this manually

+++ Updating remotes...
Fetching upstream
Failed to add the RSA host key for IP address '140.82.112.4' to the list of known hosts (/app/.ssh/known_hosts).
From github.com:mattermost/mattermost-webapp
   00baa5885..ce3a686e8  cloud      -> upstream/cloud
   142de1d1f..5a9f4cc08  master     -> upstream/master
Fetching origin
Failed to add the RSA host key for IP address '140.82.112.4' to the list of known hosts (/app/.ssh/known_hosts).
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-mattermost-webapp-#7843-upstream-cloud-1618912769
Switched to a new branch 'automated-cherry-pick-of-mattermost-webapp-#7843-upstream-cloud-1618912769'
Branch 'automated-cherry-pick-of-mattermost-webapp-#7843-upstream-cloud-1618912769' set up to track remote branch 'cloud' from 'upstream'.

+++ About to attempt cherry pick of PR #7843 with merge commit c2c4725befd6265ab90f45c054aa9340fe8787b1.

error: could not apply c2c4725be... MM-23120 Custom status issues (#7843)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

+++ Conflicts detected:

UU components/suggestion/switch_channel_provider.jsx
Aborting.

+++ Aborting in-progress git cherry-pick.

+++ Returning you to the master branch and cleaning up.

@amyblais
Copy link
Member

@Willyfrog Seems that the cherry-pick didn't work

@Willyfrog
Copy link
Contributor

yeah! I'll try to do that manually

@Willyfrog
Copy link
Contributor

actually @michelengelen can you do the cherry-picking? seems the reason for the conflict is that #7932 is not in both branches

michelengelen pushed a commit that referenced this pull request Apr 20, 2021
@manojmalik20 @michelengelen
MM-23120 Custom status issues (#7843)
36d7103 
* Fixed the MM-34095 issue

* Solved several issues from the MM-23120 epic
Solved the MM-33149 issue
Solved the MM-33195 issue
Solved the MM-33214 issue
Solved the MM-33220 issue
Solved the MM-33246 issue
Solved the MM-33433 issue
Solved the MM-34571 issue
Solved the MM-33431 issue

* Updated snapshots

* Fixed some UI issues
Removed the dot in channel header after custom status
Changed the marginLeft for the custom status emoji in switch channel modal
Changed opacity of custom status emoji on hover

* Update components/custom_status/custom_status_modal.tsx

Co-authored-by: Michel Engelen <[email protected]>

* Fixed lint error

* Fixed the styling in switch channel modal

Co-authored-by: Manoj <[email protected]>
Co-authored-by: Michel Engelen <[email protected]>

(cherry picked from commit c2c4725)
@michelengelen michelengelen mentioned this pull request Apr 20, 2021
Merged
@amyblais amyblais added Changelog/Done Required changelog entry has been written CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation Docs/Needed Requires documentation and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation labels Apr 20, 2021
chetanyakan added a commit to brightscout-alpha/mattermost-webapp that referenced this pull request Apr 21, 2021
@chetanyakan
Merge branch 'master' of github.com:mattermost/mattermost-webapp into…
e6499ac 
… cs-fix

* 'master' of github.com:mattermost/mattermost-webapp: (125 commits)
  MM-31717: Remove HTTP clustering (mattermost#7924)
  MM-35021: Only display overflow names in Avatars tooltip (mattermost#7955)
  Cypress/E2E: Fix E2E from recent Cloud test run (mattermost#7957)
  save code body, error details and screenshots if available when e2e failed (mattermost#7945)
  MM-32962/MM-32930 Dependency updates (mattermost#7929)
  take into account  menu permissions (mattermost#7792)
  Fix accesibility problems in files/message selectors in hint and RHS (mattermost#7925)
  MM-23120 Custom status issues (mattermost#7843)
  [MM-34742][MM-34743][MM-34745] - Hard coded values fixes (mattermost#7926)
  [MM-34763][MM-34744][MM-34767] - Fix UX issues with channel navigator and navbar (mattermost#7934)
  Feature/mm 34670 (mattermost#7932)
  Cypress/E2E: Fix clock, timezone, local date time specs (mattermost#7949)
  MM-35016 - Update payment screen info text (mattermost#7953)
  MM-34975 - hide payment information to free trial customers (mattermost#7950)
  Translations update from Weblate (mattermost#7943)
  Fix basedn typo (mattermost#7927)
  [MM-34551] Make button disabled for Read Only Admin (mattermost#7904)
  MM-34978 - fix contact support link in the error payment screen (mattermost#7940)
  Upgrading tests to prod (mattermost#7936)
  MM-33748 Support for new mention_count_root and msg_count_root (mattermost#7733)
  ...
prapti pushed a commit that referenced this pull request Apr 22, 2021
@manojmalik20 @michelengelen @prapti
MM-23120 Custom status issues (#7843)
b01576a 
* Fixed the MM-34095 issue

* Solved several issues from the MM-23120 epic
Solved the MM-33149 issue
Solved the MM-33195 issue
Solved the MM-33214 issue
Solved the MM-33220 issue
Solved the MM-33246 issue
Solved the MM-33433 issue
Solved the MM-34571 issue
Solved the MM-33431 issue

* Updated snapshots

* Fixed some UI issues
Removed the dot in channel header after custom status
Changed the marginLeft for the custom status emoji in switch channel modal
Changed opacity of custom status emoji on hover

* Update components/custom_status/custom_status_modal.tsx

Co-authored-by: Michel Engelen <[email protected]>

* Fixed lint error

* Fixed the styling in switch channel modal

Co-authored-by: Manoj <[email protected]>
Co-authored-by: Michel Engelen <[email protected]>
@cwarnermm cwarnermm added Docs/Not Needed Does not require documentation and removed Docs/Needed Requires documentation labels Apr 23, 2021
@mm-cloud-bot
Copy link

@manojmalik20: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

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

@Willyfrog Willyfrog Willyfrog approved these changes

@matthewbirtch matthewbirtch matthewbirtch approved these changes

@ogi-m ogi-m ogi-m approved these changes

@michelengelen michelengelen michelengelen approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone do-not-merge/release-note-label-needed Docs/Not Needed Does not require documentation
Projects
None yet
Milestone
v5.35.0
10 participants
@manojmalik20 @mattermod @esethna @matthewbirtch @ogi-m @mm-cloud-bot @amyblais @michelengelen @Willyfrog @cwarnermm