-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Warning Color Consolidations #1798
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller
Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
The warning notification seems harder to read now. Was that change intentional? |
No, not intending to make anything harder to read... I only changed any given warning text/background by one shade in either direction in order to try to normalize it across the app. But I can see that in the case you mention the background was one shade darker and the text was one shade lighter, decreasing contrast in both situations, this is not desirable. I've adjusted it a bit. Can you see if it's better? If not, how do you think we should change it? Darker text? Lighter background? |
@youngcw, can you see if my changes work better? Cheers! |
Its better. I think I'll still defer to @MatissJanis since its a visual change |
👋 Sorry for the slow review. Would you mind updating the VRT screenshots to make CI pass? The updated screenshots will make reviewing this a bit easier since I'll be able to catch most of the changes by just looking at the images instead of manually looking at them in the app. |
Hey, I tried to update VRT screenshots a bunch of times but couldn't get it to work. Realized I wasn't getting any VRT errors on my local. Re-ran checks on github and they all passed. So this one is ready to go! |
I agree, seems off. I've adjusted the background back to the darker one and changed the text areas that were experiencing poor contrast issues. Have a look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #1870 |
Fixes #1869 |
Fixes #1807 |
Fixes #1806 |
Fixes #1805 |
Consolidating and making more consistent the warning and upcoming colors.