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

Improve contrast between failed and incomplete color #5121

Merged
merged 1 commit into from May 8, 2023

Conversation

Martchus
Copy link
Contributor

@Martchus Martchus commented May 5, 2023

Commit c3af6a3 increased the contrast of progress bar segments showing the number of failures by making the color we generally use for failures darker. However, that decreased the difference between the color of failures and incompletes which makes them harder to distinguish. So this commit reverts this original change. To still have a good contrast in the progress bar segment the text color within the progress bar is set to black. This looks good in bright and dark mode (the mode actually makes no difference for the progress bars).

@github-actions
Copy link

github-actions bot commented May 5, 2023

Great PR! Please pay attention to the following items before merging:

Files matching assets/stylesheets/**:

  • Consider providing screenshots in a PR comment to show the difference visually

This is an automatically generated QA checklist based on modified files.

@Martchus
Copy link
Contributor Author

Martchus commented May 5, 2023

The progress bar now looks like this:
image

In the dark mode the colors are exactly the same so I'm saving me the screenshot here. (It is actually not that ideal that the colors are the same but that's a different topic.)

Commit c3af6a3 increased the contrast
of progress bar segments showing the number of failures by making the
color we generally use for failures darker. However, that decreased the
difference between the color of failures and incompletes which makes
them harder to distinguish. So this commit reverts this original change.
To still have a good contrast in the progress bar segment the text color
within the progress bar is set to black. This looks good in bright and
dark mode (the mode actually makes no difference for the progress bars).

Related ticket: https://progress.opensuse.org/issues/128783
@kalikiana
Copy link
Member

kalikiana commented May 5, 2023

Since this isn't just a revert, did you discuss it with @Lokeshsri11 yet? And e.g. make sure we know that black on #FF4E46 is actually a good combination.

I guess wider design concerns are no longer relevant as the project's been rejected...

@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Merging #5121 (cabb5e8) into master (bf38be7) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5121      +/-   ##
==========================================
- Coverage   98.25%   98.24%   -0.01%     
==========================================
  Files         383      383              
  Lines       36348    36348              
==========================================
- Hits        35713    35711       -2     
- Misses        635      637       +2     

see 1 file with indirect coverage changes

@perlpunk
Copy link
Contributor

perlpunk commented May 5, 2023

I retriggered the OBS checks, they looked like they failed because of some OBS internal problem (e.g. readonly file system)

@perlpunk
Copy link
Contributor

perlpunk commented May 5, 2023

Actually that didn't help:
https://build.opensuse.org/package/live_build_log/devel:openQA:GitHub:os-autoinst:openQA:PR-5121/openQA/openSUSE_Tumbleweed/x86_64

[   48s] ### VM INTERACTION START ###
[   48s] [0;1;38;5;185mFailed to write wtmp record, ignoring: Read-only file system[0m
[   48s] [0;1;38;5;185mFailed to write utmp record: Read-only file system[0m
[   48s] Powering off.
[   48s] [   43.373792][ T3275] reboot: Power down
[   48s] ### VM INTERACTION END ###

@perlpunk
Copy link
Contributor

perlpunk commented May 5, 2023

Oh, there's a different error actually:
https://build.opensuse.org/package/live_build_log/devel:openQA:GitHub:os-autoinst:openQA:PR-5121/openQA/openSUSE_Leap_15.4/x86_64
https://build.opensuse.org/package/live_build_log/devel:openQA:GitHub:os-autoinst:openQA:PR-5121/openQA/openSUSE_Tumbleweed/x86_64

[   91s] error: line 159: Dependency tokens must begin with alpha-numeric, '_' or '/': Requires:       perl(CSS::Minifier::XS) ...

@perlpunk
Copy link
Contributor

perlpunk commented May 5, 2023

#5122 should fix the OBS issue. Not sure why the error didn't popup already for #5117

@okurz
Copy link
Member

okurz commented May 5, 2023

#5121 (comment) looks like the contrast on red is a bit too good now. Do we use some kind of gray text in the other areas?

@Lokeshsri11
Copy link
Contributor

yes this one looks good

@Martchus
Copy link
Contributor Author

Martchus commented May 5, 2023

The other progress bar segments are likely the normal link color. I don't think it would look good on the red background, though. We already use black for skipped progress bar segments so I thought consistency with that would be nice. Of course we could also use the normal text color in all progress bar segments that so far use black and maybe even in all progress bar segments for the best consistency.

@Lokeshsri11
Copy link
Contributor

The other progress bar segments are likely the normal link color. I don't think it would look good on the red background, though. We already use black for skipped progress bar segments so I thought consistency with that would be nice. Of course we could also use the normal text color in all progress bar segments that so far use black and maybe even in all progress bar segments for the best consistency.

Yes, I also feel the same thing, just change the red to normal red, everything else is fine.

@mergify mergify bot merged commit 07a8834 into os-autoinst:master May 8, 2023
36 checks passed
@Martchus Martchus deleted the contrast branch May 8, 2023 08:40
@Vogtinator
Copy link
Member

At least for me, the black text on the dark red "failed jobs" bar is really hard to read.

@Martchus
Copy link
Contributor Author

Martchus commented May 8, 2023

I find it easier to read than the white text¹ although both is actually pretty well readable so I have no strong opinion on that. I also don't find the red very "dark". I suppose colors are a complicated matter :-)

¹ Which failed some contrast test (see #5080).

@Vogtinator
Copy link
Member

I suppose colors are a complicated matter :-)

Yep! I've got red-green color impairment, so that could be a reason why red appears to be much darker to me.

@okurz
Copy link
Member

okurz commented May 8, 2023

maybe we can now try to make the red a bit lighter?

@Martchus
Copy link
Contributor Author

Martchus commented May 8, 2023

We can make it a little bit lighter - but not too light. It still needs good contrast with the usually white background.

@Lokeshsri11
Copy link
Contributor

We can make it a little bit lighter - but not too light. It still needs good contrast with the usually white background.

Yes, if we do more light, then the contrast will not match on the light theme mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants