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

feat:proper badge theme use for server list #8313

Merged
merged 5 commits into from
Nov 5, 2024

Conversation

rahimrahman
Copy link
Contributor

@rahimrahman rahimrahman commented Nov 4, 2024

Summary

While fixing issue with #8312, there has been a few changes to the badge in the server list requested by @asaadmahmood.

Ticket Link

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: iPhone 15 Simulator, 17.4.1

Screenshots

denim-sapphire-quartz-indigo-onyx-after-asaad-fix-2

Release Note


@mm-cloud-bot mm-cloud-bot added kind/feature Categorizes issue or PR as related to a new feature. release-note labels Nov 4, 2024
@rahimrahman rahimrahman changed the title feat:new badge server list feat:proper badge theme use for server list Nov 4, 2024
@rahimrahman
Copy link
Contributor Author

rahimrahman commented Nov 4, 2024

For comparison, this is Quartz before fix, slightly thicker border (hardly noticeable):

quartz-fixed

@matthewbirtch
Copy link
Contributor

@rahimrahman the Denim and Sapphire themes look good in the screenshots. However, Quartz, Onyx, and Indigo aren't quite right. They should not have blue borders surrounding the badge and the badge background and number should follow the button colors.

It looks like you're using the right theme color variables in the code, but the screenshots aren't showing right. They should look like this:

image

@enahum
Copy link
Contributor

enahum commented Nov 4, 2024

@rahimrahman the Denim and Sapphire themes look good in the screenshots. However, Quartz, Onyx, and Indigo aren't quite right. They should not have blue borders surrounding the badge and the badge background and number should follow the button colors.

It looks like you're using the right theme color variables in the code, but the screenshots aren't showing right. They should look like this:

image

@matthewbirtch I have a question.
Why if the code is exactly the same for all themes, some look ok but others don't?

Cause as far as I can see there is no specific code based on the theme.

@matthewbirtch
Copy link
Contributor

@matthewbirtch I have a question.
Why if the code is exactly the same for all themes, some look ok but others don't?

Cause as far as I can see there is no specific code based on the theme.

@enahum @rahimrahman is it possible the screenshots are just old? They don't match with what is applied in code.

My comment was just based on the screenshots, but I can spin up a build and test that to confirm

@matthewbirtch matthewbirtch added the Build Apps for PR Build the mobile app for iOS and Android to test label Nov 4, 2024
@rahimrahman
Copy link
Contributor Author

rahimrahman commented Nov 5, 2024

@matthewbirtch sorry, I think I screwed up my screenshots with a different branch. This is the correct one:

denim-sapphire-quartz-indigo-onyx-after-asaad-fix-2

The 1px borderWidth change requested might need/should get reverted. It's too skinny IMO.

@rahimrahman
Copy link
Contributor Author

Back to 2px borderWidth

denim-sapphire-quartz-indigo-onyx-after-asaad-fix-2-borderWidth-2px

@rahimrahman
Copy link
Contributor Author

Monokai & Organization:

monokia-organization-after-asaad-fix-2-borderWidth-2px

Copy link

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

Looks good to me!

@hanzei hanzei removed the request for review from a team November 5, 2024 07:47
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

Codewise, LGTM.(we still need to revert the border width change, though).

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 @rahimrahman. Looks good!

Base automatically changed from fix/MM-61209-badge-server-incorrect-theme-color to main November 5, 2024 13:25
@rahimrahman rahimrahman added 3: QA Review Requires review by a QA tester and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Nov 5, 2024
@rahimrahman rahimrahman added the Build Apps for PR Build the mobile app for iOS and Android to test label Nov 5, 2024
@rahimrahman
Copy link
Contributor Author

@matthewbirtch @asaadmahmood I don't think there's much to test here other than what's already been displayed in a few screenshots.

Should we skip QA Review?

@yasserfaraazkhan yasserfaraazkhan added QA Deferred Agreement with QA that these changes will be tested post-merge and removed 3: QA Review Requires review by a QA tester labels Nov 5, 2024
@yasserfaraazkhan
Copy link
Contributor

should be good to merge. It'll be tested during code freeze.

@rahimrahman rahimrahman merged commit 97e74c9 into main Nov 5, 2024
27 of 28 checks passed
@rahimrahman rahimrahman deleted the feat/new-badge-server-list branch November 5, 2024 14:11
@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
Build Apps for PR Build the mobile app for iOS and Android to test kind/feature Categorizes issue or PR as related to a new feature. QA Deferred Agreement with QA that these changes will be tested post-merge release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants