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

Use lighter red shades for error messages #4611

Merged
merged 21 commits into from
Dec 28, 2024
Merged

Conversation

pat-s
Copy link
Contributor

@pat-s pat-s commented Dec 22, 2024

The current one has low contrast in the dark theme and is somewhat hard to look at.

The new red color represents the "500" shaded in the palette. Other colors from the generic red palette have been adjusted in the linked palette.

Old New
image image
image image
image image

@pat-s pat-s added ui frontend related build_pr_images If set, the CI will build images for this PR and push to Dockerhub labels Dec 22, 2024
@pat-s pat-s force-pushed the chore/update-error-color branch from 77117b3 to 25efdf0 Compare December 23, 2024 16:47
@pat-s pat-s marked this pull request as ready for review December 24, 2024 14:52
@xoxys
Copy link
Member

xoxys commented Dec 24, 2024

IMO this is way too "aggressive" for the eyes in dark mode and I would prefer the current desaturated variant.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 24, 2024

IMO this is way too "aggressive" for the eyes in dark mode and I would prefer the current desaturated variant.

Does this apply to both shades or only the button background? Note that the icon one is darker.

web/windi.config.ts Outdated Show resolved Hide resolved
@xoxys
Copy link
Member

xoxys commented Dec 24, 2024

There are two different red tones used for the icon and the button?

@pat-s
Copy link
Contributor Author

pat-s commented Dec 25, 2024

Two classes (there are many redundant ones with the same color but this is a different topic) but only one tone: #c93c37. (I tried using different ones in a previous state but the current one only uses one color).

It would be great if there could be additional opinions as it is hard to move on in such UI discussions if there's always a 1:1.

(just as a side note: I have found the red color being way too dark since the theme existed, this is not a recent discovery. I have also compared the WP color with other common red colors in dark themes (GH, GL, Hetzner) and none is nowhere close in terms of darkness/saturation.
Not saying that my proposed choice is a good alternative but I really think the current one is sub-optimal.)

@pat-s
Copy link
Contributor Author

pat-s commented Dec 25, 2024

#4584 (comment) - guess I'll have to close this PR?

@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

I dont think so, Im fine with a color change.

@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

Im also fine with the not-desaturated color, just dont had time to come back to this PR.

@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

The desaturated looks now more pink in dark mode. Then I would prefer your initial one.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 25, 2024

The desaturated looks now more pink in dark mode. Then I would prefer your initial one.

Still trying. Iterating in the PR, not locally.
Adding saturation wasn't a good idea.

@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

Ok, sorry. Hard to see what is "developing commit" and whats not. Is there a reason not to use a local compose stack to test your changes? Just wondering as for me the feedback loop would be way too long otherwise.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 25, 2024

Ok, sorry. Hard to see what is "developing commit" and whats not. Is there a reason not to use a local compose stack to test your changes? Just wondering as for me the feedback loop would be way too long otherwise.

When you iterate on 4+ PRs, letting the CI build everything and then just kill the pod is the simplest way imo (my dev instance is on k8s). I sometimes build local images but only until I am convinced this is worth a PR and the CI build actually working (and it is not a UI PR).

I think building UI PR images publicly is important as others can easily apply the images this way and verify changes in detail.

@pat-s pat-s marked this pull request as draft December 25, 2024 22:08
@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

image
image

I just tested colors.red[500] and IMO it is pretty close to the custom color you selected. I Would just use this one for the dark mode and drop the desaturation.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 25, 2024

I just tested colors.red[500] and IMO it is pretty close to the custom color you selected. I Would just use this one for the dark mode and drop the desaturation.

Hm, that's how I started out initially but wasn't convinced...but ok, I'll try again.

@xoxys
Copy link
Member

xoxys commented Dec 25, 2024

Yeah, it is not exactly the same color of course, but Im wondering if the minimal difference is really important. Btw we have also introduced theming by custom css to adjust such things to personal preferences without code changes.

@pat-s pat-s force-pushed the chore/update-error-color branch from 1bc59da to 47ff500 Compare December 25, 2024 22:22
@pat-s pat-s requested a review from xoxys December 25, 2024 23:05
@pat-s pat-s marked this pull request as ready for review December 25, 2024 23:05
@pat-s
Copy link
Contributor Author

pat-s commented Dec 25, 2024

OK, this one works for me. Updated the screenshot in OP.

@xoxys
Copy link
Member

xoxys commented Dec 26, 2024

Would wp-state-error-200 instead of wp-state-error-100 in dark mode work for you as well? Can we also remove 400: 'var(--wp-state-error-400)' and 500: 'var(--wp-state-error-500)', for wp-state-error again? It's not uses anywhere.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 26, 2024

Can we also remove 400: 'var(--wp-state-error-400)' and 500: 'var(--wp-state-error-500)', for wp-state-error again?

The idea that was to have a larger palette defined for potential future uses that allows colors below and above the current active spectrum. (Yes, I know that the "below" part is actually missing). AFAIK there are also not all green primary colors in use which are defined?
But to move on and to avoid further discussions, I'll remove them.

Would wp-state-error-200 instead of wp-state-error-100 in dark mode work for you as well?

OK.

@pat-s
Copy link
Contributor Author

pat-s commented Dec 27, 2024

@xoxys Your turn again.

Copy link
Member

@xoxys xoxys left a comment

Choose a reason for hiding this comment

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

Just a nit. Maybe we should also rename wp-state-error to wp-error as we now use it in multiple different locations? Can also be done in a separate PR.

web/src/style.css Outdated Show resolved Hide resolved
web/src/style.css Outdated Show resolved Hide resolved
@woodpecker-bot
Copy link
Contributor

woodpecker-bot commented Dec 27, 2024

Deployment of preview was torn down

@pat-s pat-s requested a review from xoxys December 27, 2024 22:19
@xoxys
Copy link
Member

xoxys commented Dec 27, 2024

@pat-s there are open suggestions

@xoxys
Copy link
Member

xoxys commented Dec 27, 2024

There are still a lot of wp-state-error occurrences that were not renamed properly.

@xoxys xoxys merged commit bf75119 into main Dec 28, 2024
7 checks passed
@xoxys xoxys deleted the chore/update-error-color branch December 28, 2024 14:26
@woodpecker-bot woodpecker-bot mentioned this pull request Dec 28, 2024
1 task
@anbraten anbraten added the enhancement improve existing features label Jan 18, 2025
@anbraten anbraten changed the title Add lighter red shades for error messages Use lighter red shades for error messages Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build_pr_images If set, the CI will build images for this PR and push to Dockerhub enhancement improve existing features ui frontend related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants