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

fix(MM-61209): mention badge incorrect theme color #8312

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

rahimrahman
Copy link
Contributor

@rahimrahman rahimrahman commented Nov 4, 2024

Summary

Most noticeable in the quartz theme, the mention badge and unread badge is using the wrong colors. See attached. Because of this the mention badge is nearly invisible.

IMG_4282

Fix, what it looks like on Quartz:

quartz-fixed

Ticket Link

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

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on:

Screenshots

Release Note


@mm-cloud-bot mm-cloud-bot added kind/bug Categorizes issue or PR as related to a bug. release-note labels Nov 4, 2024
@rahimrahman rahimrahman requested review from asaadmahmood, matthewbirtch, a team, Rajat-Dabade and devinbinnie and removed request for a team November 4, 2024 17:54
@rahimrahman
Copy link
Contributor Author

rahimrahman commented Nov 4, 2024

@asaadmahmood @matthewbirtch notice the after fix shows the border on the badge in the server list has changed to blue (for Onyx).

Onyx before fix:

onyx-before-fix

Onyx after:

onyx-fixed

@asaadmahmood
Copy link

@rahimrahman Maybe Matt knows, but shouldn't the mention badge be like the mention badge on the team?
382867807-c23c32de-311c-4449-a682-eb05acde2ae3

@rahimrahman
Copy link
Contributor Author

@asaadmahmood @matthewbirtch

IMO, that would be a different problem/scope. Right now, my goal is to fix the issue with badge having the same border and background color within the server list component. If we're changing the UI, we absolutely can do that too, but I think that should be a different ticket. And probably should have the proper figma representation of the mobile design guide as well, particularly for server list from the bottom sheet.

But what I want to showcase as well, by fixing the problem with quartz, the border for Onyx is now different color (in the server list), because in the server list, it became blue instead of black.

And I could pull all other themes, and probably will resemble the same result.

@asaadmahmood
Copy link

asaadmahmood commented Nov 4, 2024

@rahimrahman I'm not suggesting any major UI changes.
Can we just instead use buttonBg as the background for the badge, and buttonTextColor for the badge text color.
And the border can be the same color as the background of the sheet item.

@rahimrahman
Copy link
Contributor Author

@asaadmahmood I'll create a new PR for this and showcase all the different themes.

What is the (closest) theme used in your figma file?

@enahum
Copy link
Contributor

enahum commented Nov 4, 2024

Can we make sure this works well with all the themes? And maybe even some custom themes out there?

This issue has been brought forward a few times (in the last 4 years) so perhaps we should look at it in depth.

As for example why is using buttonBg instead of mentionBg etc..

Well this are my two cents

@rahimrahman rahimrahman force-pushed the fix/MM-61209-badge-server-incorrect-theme-color branch from bcd7fdc to 0c98f9e Compare November 4, 2024 21:33
const {backgroundColor = rest.backgroundColor || theme.mentionBg, ...restStyle} =
const {

// @ts-expect-error: backgroundColor & borderColor definitely exist
Copy link
Member

Choose a reason for hiding this comment

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

Should we be asserting here instead of expecting an error? 0/5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All we care about are two of the TextStyle props: backgroundColor & borderColor so that we can pass into the the Animated.Text component. I think it is safe to cast the flatten() as TextStyle.

@rahimrahman
Copy link
Contributor Author

rahimrahman commented Nov 4, 2024

Can we make sure this works well with all the themes? And maybe even some custom themes out there?

This issue has been brought forward a few times (in the last 4 years) so perhaps we should look at it in depth.

As for example why is using buttonBg instead of mentionBg etc..

Well this are my two cents

@enahum @asaadmahmood I purposely created a new PR. There is a bug here where if devs added both props badgeStyle (which contains borderColor) and borderColor, then whatever in the badgeStyle has higher priority than borderColor. It so happens that the badgeStyle has a borderColor of '#ffffff' and the actual borderColor has the right value of '#1c58d9'. Thus why the badge has the incorrect white border.

For the follow-up discussion on theme key changes for server list badge (bottom sheet), please see the other PR.

@rahimrahman rahimrahman added 3: QA Review Requires review by a QA tester Build Apps for PR Build the mobile app for iOS and Android to test labels Nov 5, 2024
@rahimrahman
Copy link
Contributor Author

@yasserfaraazkhan there is a lot of noise in this PR about changing how the theme configuration should apply on the server list badge. The purpose of this PR is a bug fix in which in quartz, whatever being passed in borderColor is being overwritten by the borderColor prop within badgeStyle.

I believe borderColor should always take priority, that's what is happening in this PR. There is another PR that the design team will weigh in on what is the appropriate theme keys to be used for the server list badge.

Copy link
Contributor

@yasserfaraazkhan yasserfaraazkhan left a comment

Choose a reason for hiding this comment

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

Thank you Rahim.
I added 2 teams and verified for all themes that mentioned badge and unread (like a DM) are displaying in the add server modal.

Tested on Android and iOS device

@rahimrahman rahimrahman added QA Review Done and removed 3: QA Review Requires review by a QA tester Build Apps for PR Build the mobile app for iOS and Android to test labels Nov 5, 2024
@rahimrahman rahimrahman merged commit aa85f22 into main Nov 5, 2024
28 checks passed
@rahimrahman rahimrahman deleted the fix/MM-61209-badge-server-incorrect-theme-color branch November 5, 2024 13:25
@amyblais amyblais added this to the v2.23.0 milestone Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. QA Review Done release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants