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

[MM-23414] Fix up multiselect styling #5087

Merged
merged 5 commits into from
Mar 25, 2020

Conversation

fmunshi
Copy link
Contributor

@fmunshi fmunshi commented Mar 19, 2020

Summary

  • The original issue was just that the dark mode styles were too dark - the cause of which i traced back to MM-22873 - Updating input UI in DM modal #5011
  • Turns out the select input of this modal never modified its colours to match the current theme so I made some changes to its styles to look good in both dark and light themes
  • A lot of the styles were borrowed heavily from /components/widgets/inputs/user_emails_input.scss (from the invite team members select list)

Ticket Link

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

Before

Screen Shot 2020-03-19 at 5 01 19 PM
Screen Shot 2020-03-19 at 5 00 59 PM

After

Screen Shot 2020-03-19 at 5 02 11 PM
Screen Shot 2020-03-19 at 5 02 03 PM

@fmunshi fmunshi added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester 1: UX Review Requires review by a UX Designer labels Mar 19, 2020
@fmunshi fmunshi force-pushed the MM-23414-Fix-direct-message-colour branch from e03ab55 to b4c955e Compare March 19, 2020 21:06
@asaadmahmood
Copy link
Contributor

asaadmahmood commented Mar 20, 2020

@fm2munsh I submitted a PR to fix the dark theme issue and the height.
I think we can still take the rounded thing and the stylistic changes from this PR.
See if you can resolve the conflicts.
#5079

Copy link
Contributor

@asaadmahmood asaadmahmood left a comment

Choose a reason for hiding this comment

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

See comment. And I think we can keep the react-select class and react-select-auto class.

Copy link
Member

@jwilander jwilander 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 added this to the v5.22.0 milestone Mar 20, 2020
@amyblais amyblais added the CherryPick/Approved Meant for the quality or patch release tracked in the milestone label Mar 20, 2020
@fmunshi
Copy link
Contributor Author

fmunshi commented Mar 20, 2020

@asaadmahmood Thanks - I didnt realize you already had a fix for this - I merged my changes with yours and kept the styling changes I made
Screen Shot 2020-03-20 at 10 34 46 AM

@fmunshi fmunshi requested review from asaadmahmood and removed request for michaelgamble March 20, 2020 14:42
Copy link
Contributor

@asaadmahmood asaadmahmood left a comment

Choose a reason for hiding this comment

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

Awesome, thank you.

@fmunshi fmunshi removed 1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core commiter labels Mar 20, 2020
@lindalumitchell lindalumitchell added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Mar 25, 2020
Copy link
Contributor

@lindalumitchell lindalumitchell left a comment

Choose a reason for hiding this comment

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

Tested with the four in-app default themes, two of the custom themes from https://docs.mattermost.com/help/settings/theme-colors.html#custom-theme-examples, and using the color pickers to create a new custom theme. No issues found on that selector or others that I looked at, including the invite people UI as mentioned above. LGTM!

@lindalumitchell lindalumitchell added QA Review Done and removed 3: QA Review Requires review by a QA tester labels Mar 25, 2020
@cpanato cpanato removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Mar 25, 2020
@mattermod
Copy link
Contributor

Test server destroyed

@fmunshi fmunshi added the AutoMerge used by Mattermod to merge PR automatically label Mar 25, 2020
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@fmunshi fmunshi merged commit 19e7af0 into mattermost:master Mar 25, 2020
@fmunshi fmunshi deleted the MM-23414-Fix-direct-message-colour branch March 25, 2020 15:44
@mattermod
Copy link
Contributor

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

+++ Updating remotes...
Fetching upstream
Fetching origin
+++ Updating remotes done...
+++ Creating local branch automated-cherry-pick-of-#5087-upstream-release-5.22-1585151061
Branch 'automated-cherry-pick-of-#5087-upstream-release-5.22-1585151061' set up to track remote branch 'release-5.22' from 'upstream'.
+++ Downloading patch to /tmp/5087.patch (in case you need to do this again)

+++ About to attempt cherry pick of PR. To reattempt:
  $ git am -3 /tmp/5087.patch

Applying: Improve direct messages multiselect styling
Using index info to reconstruct a base tree...
M	components/multiselect/__snapshots__/multiselect.test.tsx.snap
M	components/multiselect/multiselect.tsx
Falling back to patching base and 3-way merge...
Auto-merging components/multiselect/multiselect.tsx
CONFLICT (content): Merge conflict in components/multiselect/multiselect.tsx
Auto-merging components/multiselect/__snapshots__/multiselect.test.tsx.snap
CONFLICT (content): Merge conflict in components/multiselect/__snapshots__/multiselect.test.tsx.snap
Patch failed at 0001 Improve direct messages multiselect styling
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

+++ Conflicts detected:

UU components/multiselect/__snapshots__/multiselect.test.tsx.snap
UU components/multiselect/multiselect.tsx

+++ Aborting in-progress git am.

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

@fmunshi fmunshi removed the AutoMerge used by Mattermod to merge PR automatically label Mar 25, 2020
fmunshi added a commit to fmunshi/mattermost-webapp that referenced this pull request Mar 25, 2020
* Improve direct messages multiselect styling

* Re-add react-select class

* Extra quote
@fmunshi fmunshi added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Mar 25, 2020
fmunshi added a commit that referenced this pull request Mar 25, 2020
* Improve direct messages multiselect styling

* Re-add react-select class

* Extra quote
@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Mar 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Changelog/Not Needed Does not require a changelog entry CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone Docs/Not Needed Does not require documentation QA Review Done
Projects
None yet
7 participants