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

[#51275] Fix Primer checkboxes lacking a background in High Contrast Mode #14267

Merged
merged 2 commits into from
Nov 29, 2023

Conversation

aaron-contreras
Copy link
Contributor

@aaron-contreras aaron-contreras commented Nov 28, 2023

Description

This was setting Primer's CSS variable to one of our variables. As a side effect, this fix is two fold:

  1. Sets the right background color on checkboxes in High Contrast Mode.
  2. Sets Links to the default color as indicated in the Primer Lookbook while in High Contrast Mode.

Notes

I'm a bit torn on whether the links should be black or blue while in High Contrast Mode. According to the Primer Lookbook, links have the following schemes while in High Contrast Mode:

Primer color schemes for links

I had a talk with @bsatarnejad about this and the black color on links seems to be intentional. This would mean that the "primary" style might be the desired scheme to use by default. I see the default is intended to be blue according to the lookbook.

See: https://community.openproject.org/work_packages/51275

Copy link
Contributor

@HDinger HDinger left a comment

Choose a reason for hiding this comment

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

Hi @aaron-contreras , @bsatarnejad

I'm a bit torn on whether the links should be black or blue while in High Contrast Mode. According to the Primer Lookbook, links have the following schemes while in High Contrast Mode:

I had a talk with @bsatarnejad about this and the black color on links seems to be intentional. This would mean that the "primary" style might be the desired scheme to use by default. I see the default is intended to be blue according to the lookbook.

If the primer default is blue, that should be the default for us as well, imho. @bsatarnejad maybe we can have a short chat about this?

Regarding the current solution: The reason for overriding color-accent-fg with our link-color was to make sure that all links in the application have the currently configured design color. Without that, we have different link colors depending on whether our variable (content-link-color) or the Primer one (color-accent-fg) is used:

Bildschirmfoto 2023-11-29 um 07 43 57

In my eyes, the actual problem appears because we have two conflicting rules

 --color-accent-fg: var(--content-link-color) !important;

and

--content-link-color: var(--color-accent-fg);

Basically they are overiding each other in a loop, which the browser doesn't know to resolve correctly. So my suggested solution would be:

 /* Overwrite with Primer primitives */
 [data-color-mode] {
     --body-background: var(--color-canvas-default);
     --body-font-color: var(--color-fg-default);
 }
 /* Only in the normal mode, the link color of Primer is overwritten
  * to ensure compatibility with the currently configured design. */
 [data-color-mode]:not([data-light-theme=light_high_contrast]) {
     --color-accent-fg: var(--content-link-color) !important;
 }
 [data-light-theme=light_high_contrast] {
   ... 

Edit: This would have the effect, that the checkboxes (in normal mode) have the same background as the links. So if the custom design configured yellow as the link color, the checkboxes would have a yellow background as well. I am not quite sure, if we want to accept that. If not, we'd have to override --control-checked-bgColor-rest to any wanted value in the normal mode.

@HDinger
Copy link
Contributor

HDinger commented Nov 29, 2023

Edit: This would have the effect, that the checkboxes (in normal mode) have the same background as the links. So if the custom design configured yellow as the link color, the checkboxes would have a yellow background as well. I am not quite sure, if we want to accept that. If not, we'd have to override --control-checked-bgColor-rest to any wanted value in the normal mode.

I discussed that with the product team, and we accept that behaviour for now. I created a ticket to track that: https://community.openproject.org/projects/openproject/work_packages/51313/activity

This was setting Primer's CSS variable to one of our variables. The fix
is two fold:

1. Sets the right background color on checkboxes in High Contrast Mode.
2. Sets Links to the default color as indicated in the Primer Lookbook
   while in High Contrast Mode.
This ensures consistent rendering color of links when having custom
colors set up on the instance.
@aaron-contreras aaron-contreras force-pushed the bug/51275-high-contrast-primer-checkboxes branch from fc8a2f7 to 755c326 Compare November 29, 2023 13:49
@aaron-contreras
Copy link
Contributor Author

Thanks @HDinger . Addressed in 755c326

@aaron-contreras aaron-contreras merged commit e74be7a into dev Nov 29, 2023
11 checks passed
@aaron-contreras aaron-contreras deleted the bug/51275-high-contrast-primer-checkboxes branch November 29, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants