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

feature/7573-UpdateAppColorsToComponentLibrary #9199

Merged
merged 33 commits into from
Nov 14, 2024

Conversation

theodur
Copy link
Contributor

@theodur theodur commented Jul 25, 2024

Description of Change

Migrates the colors in the app to use the colors from the component library. Also removes unused color schemes from the repo.

Screenshots/Video

To many changes to show 😅

Testing

  • Tested on iOS
  • Tested on Android

Reviewer Validations

Verify colors in the app have been migrated to use the component library colors wherever applicable.

PR Checklist

Reviewer: Confirm the items below as you review

  • PR is connected to issue(s)
  • Tests are included to cover this change (when possible)
  • No magic strings (All string unions follow the Union -> Constant type pattern)
  • No secrets or API keys are checked in
  • All imports are absolute (no relative imports)
  • New functions and Redux work have proper TSDoc annotations

For QA

Run a build for this branch

@theodur theodur marked this pull request as ready for review July 30, 2024 02:55
@theodur theodur requested review from a team as code owners July 30, 2024 02:55
cadibemma
cadibemma previously approved these changes Jul 31, 2024
@github-actions github-actions bot added FE-With QA A PR waiting for QA Activity and removed FE-Needs Review labels Jul 31, 2024
@TKDickson TKDickson added FE-Changes Requested Requested changes from Eng or QA and removed FE-With QA A PR waiting for QA Activity labels Aug 12, 2024
@theodur theodur requested a review from brea11y November 11, 2024 22:04
@brea11y
Copy link
Contributor

brea11y commented Nov 12, 2024

Hey @theodur

I'm not really sure why the new message indicator icon was being so glitchy, but everything looks great now in both dark and light mode. For some reason, that is the only area of the app that will not change without completely closing out the app. To force it to change to the right color, I have to close out of the app, switch the color mode from dark to light (or vice versa) and then launch the app again to see the change. For everything else, I can actually just change the color mode setting while I have the app open. However, since Veterans are likely never going to do that, I think this is fine. In the future, if we do decide to add the accessibility controls in the app for dark / light mode, we may want to circle back and see why this particular element isn't adjusting without completely closing out the app, but I think we're good for now.

I also double-checked the move menu / picker list in dark mode and in light mode (on Android and iOS even though Android was already showing properly) and it looks right on my end!

Since we are not going to be making adjustments to the icons in this ticket and will be revisiting those in another ticket, I'd say we should be good to wrap up VQA.

@theodur
Copy link
Contributor Author

theodur commented Nov 12, 2024

@brea11y Thanks for confirming that we're all set! I took a look at production, and confirmed the same issue is happening there, with the unread message circle not adapting to the device's theme changing. This is due to the circle icon being rendered with the inbox list. So in order for the icon color to update, the inbox list would need to be updated, which explains why reopening the app displays the correct color. I agree that it's not likely that a user would be doing that mid app session, and even so, probably wouldn't notice. I can still make a bug ticket to look into it though.

Also, when I did one final run through of the changes, I noticed the primary text color was off from one of my past commits. It was displaying the primary text in blue:

Screenshot

Simulator Screenshot - iPhone 13 - 2024-11-12 at 12 35 04

I pushed a commit to fix that, so now the primary colors are normal again. Not sure whether you wanted to do another pass at VQA, but just wanted to let you know. Once I get this PR reapproved, I'll move this to With QA since everything looks good.

@theodur theodur removed FE-Changes Requested Requested changes from Eng or QA FE-Needs Review labels Nov 12, 2024
@brea11y
Copy link
Contributor

brea11y commented Nov 12, 2024

@theodur – Thanks so much for fixing this! I noticed the text color change in light mode, but assumed it was something else that had been requested as an update with another ticket. Things are constantly changing and I just figured someone else had requested that update and it was worked into these changes. 🤣

I'll go ahead and run another build and do another look through all of the screens (including the onboarding carousel), just to make sure that there is nothing else that we missed and let you know ASAP. I don't anticipate any additional updates, but agree that it is better to take a last looks before we pass it on to QA.

@theodur
Copy link
Contributor Author

theodur commented Nov 12, 2024

Thanks so much for fixing this! I noticed the text color change in light mode, but assumed it was something else that had been requested as an update with another ticket. Things are constantly changing and I just figured someone else had requested that update and it was worked into these changes. 🤣

@brea11y That's what I thought at first too 😂 but when I did the comparison, I was like, "wait a minute..." lol

I'll go ahead and run another build and do another look through all of the screens (including the onboarding carousel), just to make sure that there is nothing else that we missed and let you know ASAP. I don't anticipate any additional updates, but agree that it is better to take a last looks before we pass it on to QA.

I agree, that sounds good. Let me know if you notice anything!

@brea11y
Copy link
Contributor

brea11y commented Nov 12, 2024

@theodur – I found one new issue, but other than that, everything looks great on my end.

Unfortunately, now the bullets in the onboarding carousel are not the right color in light mode. They are right in dark mode, but since we use the same colors in both dark and light mode for that carousel, let's make sure that both color modes are pulling in the bullet color that is currently being used for dark mode.

Light mode (incorrect)

Screenshot_20241112_171236_VA

Dark mode (correct)

Screenshot_20241112_172131_VA

The only other thing that I want to circle back to one more time is the icon colors in dark mode. You confirmed that we are using those new VADS / Design System icons (THANK YOU for that), but that they have been manipulated to still look like the duotone icons which have a different color icon in the center and a different color circle.

It sounded like it was decided by the global team that we would circle back and remove what was done to them to manipulate them in another/future ticket. However, was it also decided that we wouldn't at least update them for now so that they look correct in dark mode? I only ask because if we leave them as is and don't update them from what is currently in the branch, they (mainly the blue icons) are going to violate some major color contrast WCAG guidance because the white icons are barely visible against the light blue circles. They will also look very mismatched with the icons that are correct in dark mode. This feels very sloppy, especially compared to how things look in light mode.

I am totally on board with revisiting the icons in the future to remove what was done to manipulate them into having the icon filled in with another color, but I don't agree that we should just leave them as is in dark mode for now. I'm including some screenshots to show the icon issues below and also including one that shows the correct usage of the icons so that you get a sense of the correct usage vs. the incorrect usage and how sloppy and wrong it feels, especially when are using both right now.

Screenshots of some of the problem icons

Screenshot_20241112_172008_VA

Screenshot_20241112_172226_VA

Screenshot_20241112_172339_VA

Screenshot_20241112_172309_VA

Screenshot_20241112_172808_VA

Screenshot_20241112_172845_VA

Screenshot_20241112_172533_VA

Screenshot that shows some of the icons that are correct in dark mode

Screenshot_20241112_172452_VA

My recommendation is that we go ahead and update the white color that is currently being used to fill them in dark mode with vads-color-base-darkest until we can circle back and remove the fill color in the future. To me, this would be the best thing for accessibility and a good stepping stone until we can revisit them and fix them the right way in a future ticket.

@theodur
Copy link
Contributor Author

theodur commented Nov 13, 2024

Unfortunately, now the bullets in the onboarding carousel are not the right color in light mode. They are right in dark mode, but since we use the same colors in both dark and light mode for that carousel, let's make sure that both color modes are pulling in the bullet color that is currently being used for dark mode.

@brea11y Interestingly, this is how it is in prod right now too, which isn't correct. I think this change was missed in one of the recent icon tickets, most likely #9780. Even though it's not caused by the changes in this PR, I went ahead and made some changes to fix it; should be correct now ✅

In regard to the icons with contrast issues, I'm not sure why your icons are displaying like that, I just ran this branch on both iOS and Android and the icons appear the same as they do in production 🤔. This is what I'm seeing when I run the changes from this branch.

Android Crisis Line screenshot

Screenshot_1731456210

iOS Crisis Line screenshot

Simulator Screenshot - iPhone 13 - 2024-11-12 at 16 03 51

Contact VA screenshot

Simulator Screenshot - iPhone 13 - 2024-11-12 at 16 03 57

Are you only noticing this issue on your Android device? I definitely agree that the contrast like that wouldn't be ideal, and also wouldn't be fulfilling the AC's of this ticket (to maintain parity with colors in production). Just not sure why they're displaying like that for you.

@TKDickson
Copy link
Contributor

TKDickson commented Nov 13, 2024

Hey @brea11y @theodur - there's already a ticket for the onboarding carousel bullet contrast, and I think there's a fix in testable state anyways. No need to include in this PR. 10135

@theodur
Copy link
Contributor Author

theodur commented Nov 13, 2024

Hey @brea11y @theodur - there's already a ticket for the onboarding carousel bullet contrast, and I think there's a fix in testable state anyways. No need to include in this PR. 10135

@TKDickson Thanks for calling that out, didn't even realize. I already pushed a fix to my branch yesterday, I guess I'll revert that commit

@brea11y
Copy link
Contributor

brea11y commented Nov 13, 2024

@theodur – I had already run the build before you said that you'd revert the bullet back because of the other ticket. So, I do want to say that it looks great now (even though I know you'll be reverting it back).

Ugh.. Sorry, my brain is completely fried this week. I meant to include in my feedback yesterday that the icon issue is only occurring on Android. On iOS, I see what you are showing in your screenshots. I went ahead and created that new build today to check the bullet and take another look at the icons in iOS and Android. On iOS, I see the icons without the circles (except for the checkmark icons and those are the least problematic and not a huge issue) like you do, which is exactly how they should look. but even on that new build that I just created, the icons have the circles on Android and look like the screenshots I posted yesterday. In light mode, they have the darker shade of blue and white icons (which would be fine if it had to be that way) and in dark mode they have the light / pale blue circle and white icons. I even tried uninstalling the app completely and re-downloaded it from App Tester. I'm not sure what is going on and why they would be completely different?

brea11y
brea11y previously approved these changes Nov 13, 2024
@github-actions github-actions bot added the FE-With QA A PR waiting for QA Activity label Nov 13, 2024
@brea11y
Copy link
Contributor

brea11y commented Nov 13, 2024

This is approved on my end!

Note for QA: We did find a bug with the icons on Android. After doing some troubleshooting this afternoon with Theo, it appears that some Android users could see the old icons instead of the new icons (without the circle). My test device falls into that group, which is why things were looking so wonky on my end. Our hope is that the other ticket to update the icons and remove the duotone colors will resolve this issue when we get to it.

Huge thanks again @theodur for all of your hard work on this. I know it hasn't been easy and I really appreciate everything that you've done!

dumathane
dumathane previously approved these changes Nov 13, 2024
@theodur theodur dismissed stale reviews from brea11y and dumathane via e6c7248 November 14, 2024 19:35
@github-actions github-actions bot added FE-Ready to Merge and removed FE-With QA A PR waiting for QA Activity labels Nov 14, 2024
@theodur theodur merged commit a951922 into develop Nov 14, 2024
81 of 84 checks passed
@theodur theodur deleted the feature/7573-UpdateAppColorsToComponentLibrary branch November 14, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants