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

Button text does not adjust between light and dark mode #935

Closed
Tracked by #895
KrooshalUX opened this issue Jul 28, 2023 · 7 comments · Fixed by #981
Closed
Tracked by #895

Button text does not adjust between light and dark mode #935

KrooshalUX opened this issue Jul 28, 2023 · 7 comments · Fixed by #981
Assignees

Comments

@KrooshalUX
Copy link
Contributor

KrooshalUX commented Jul 28, 2023

In current light & dark mode - button text adjusts accordingly - however in next - the button text appears dark in light mode.

Is this a bug with Next or do we need to adjust ouiColorPrimary to fix any issues with the contrast function?

@BSFishy
Copy link
Contributor

BSFishy commented Aug 14, 2023

This is caused by this line:

$fillTextColor: chooseLightOrDarkText($color, $ouiColorGhost, $ouiColorInk);

Essentially, what's happening is that it thinks those colors are dark mode when it's light mode because of the contrast ratio calculation:

@function chooseLightOrDarkText($background, $lightText, $darkText) {
$lightContrast: contrastRatio($background, $lightText);
$darkContrast: contrastRatio($background, $darkText);
@if ($lightContrast > $darkContrast) {
@return $lightText;
} @else {
@return $darkText;
}
}

(basically it just chooses the color with the higher contrast ratio)

This means we'll need to adjust the colors for the buttons to render correctly (primary and warning)

@BSFishy BSFishy moved this from Todo to UX Required in Look & Feel Aug 14, 2023
@KrooshalUX
Copy link
Contributor Author

That was my guess - the thing that's driving me crazy is in testing from the color selection light text on the button color passes contrast tests on my end. I will try to adjust the primary as soon as possible to see if that fixes it with the compiler.

@joshuarrrr
Copy link
Member

Just to add some context from the current theme. The default $ouiColorPrimary is#006BB4.

$ouiColorPrimary: #006BB4 !default;

In the current dark mode overrides, that is adjusted to the lighter #1BA9F5

$ouiColorPrimary: #1BA9F5;

In the next theme, the light mode $ouiColorPrimary is #159D8D

$ouiColorPrimary: #159D8D !default;

And in next dark, it's also #159D8D

$ouiColorPrimary: #159D8D;

Note that the chooseLightOrDaskText effectively defaults to dark text - it only uses light text if it increases contrast ratio, even if both contrast ratios are sufficiently high.

Given this, it seems like our preference would be:

  1. In next theme, $ouiColorPrimary is the same in both light and dark modes (already implemente)
  2. When styling filled primary buttons, always use light text ($ouiColorGhost)

We can accomplish 2. specifically by adding some conditional logic that doesn't bother calling chooseLightOrDarkText for primary filled buttons. Note that this wouldn't change behavior for other color filled buttons. If we wanted a more widespread change to filled button text color, we should either refactor chooseLightOrDarkText or write a new function that actually encodes the desired design guidance.

@kroosh does that assessment sound right to you? I don't think we should let the internal implementation of filled buttons dictate what color you pick for primary.

@KrooshalUX
Copy link
Contributor Author

KrooshalUX commented Aug 16, 2023

" Note that this wouldn't change behavior for other color filled buttons." -what other color filled buttons are there?

I also dont want to change the primary color. I did not realize that button color primary was being updated in the Light/Dark for current - that sounds kind of sketchy to keep trying to chase all these transformations.

With the font weight having increased on buttons as per #807 , the contrast for light text on the primary color is correct. I dont like that its choosing the higher version even if , as you stated, the desired use is sufficiently high enough.

I reached out to @BSFishy with some of these approach shifts as well, so if this is good with you guys, its good with me. The dark text on the primary may be registering higher mathmatically, but its not perceptually correct.

@joshuarrrr
Copy link
Member

I also dont want to change the primary color. I did not realize that button color primary was being updated in the Light/Dark for current - that sounds kind of sketchy to keep trying to chase all these transformations.

No that's not what's happening. The primary color is not transformed. It's just used to determine which text color to use. I just meant we should be able to get the behavior we want regardless of the choice of primary. But also to note that the next theme's approach of the same primary in both modes was a little different.

@joshuarrrr
Copy link
Member

We can go ahead with the simplest fix first, and see the outcome on all the filled button styles: https://oui.opensearch.org/1.2/#/navigation/button

@joshuarrrr joshuarrrr self-assigned this Aug 16, 2023
@joshuarrrr joshuarrrr moved this from UX Required to In Progress in Look & Feel Aug 16, 2023
@joshuarrrr joshuarrrr moved this from In Progress to UX Required in Look & Feel Aug 16, 2023
@joshuarrrr joshuarrrr moved this from UX Required to In Review in Look & Feel Aug 16, 2023
@BSFishy BSFishy linked a pull request Aug 23, 2023 that will close this issue
7 tasks
@joshuarrrr
Copy link
Member

fixed by #981

@github-project-automation github-project-automation bot moved this from In Review to Done in Look & Feel Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
3 participants