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

[EuiDescriptionList] Low contrast in inline mode #6856

Closed
ryankeairns opened this issue Jun 20, 2023 · 9 comments
Closed

[EuiDescriptionList] Low contrast in inline mode #6856

ryankeairns opened this issue Jun 20, 2023 · 9 comments
Assignees
Labels
accessibility low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed

Comments

@ryankeairns
Copy link
Contributor

Describe the problem
During a cursory review of dark modes styles in Kibana, I noticed that the inline mode of EuiDescriptionList does not meet minimum contrast levels. The same WCAG failure can be seen in the EUI docs example as seen below.

Screen Shot 2023-06-20 at 4 14 04 PM

To Reproduce

  1. Start up / log in to a Kibana instance (install sample data, if needed)
  2. In the upper right, go to Edit Profile and change to 'Dark' mode
  3. Navigate to Discover and see the result below
Screen Shot 2023-06-20 at 4 18 12 PM

Proposed solution

Set the text color on the dt to text color when in dark mode.

WCAG or Vendor Guidance (optional)

@cee-chen cee-chen added the low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed label Jun 26, 2023
@kyrspl
Copy link
Contributor

kyrspl commented Aug 21, 2023

@ryankeairns - Here I'm using

background-color: ${euiTheme.colors.fullShade};
color: ${euiTheme.colors.ink};

Image

It's the last option in EUI doc

Image

It seems like euiTheme.colors.text computes to #DFE5EF in dark mode.

What do you think?

@ryankeairns
Copy link
Contributor Author

ryankeairns commented Aug 21, 2023

🤔 I was envisioning a more subtle (i.e. less contrasting) background color. In other words to use a color that is just a shade or two above the page background color with the text then being light. I have not tested the contrast values of such an approach, but I do wonder about the high contrast white bg value (above) when it is repeated extensively in an area like Discover.

Update: it would need some finessing, but #343731 meets the contrast check though it may be too faint

CleanShot 2023-08-21 at 11 59 58@2x

@kyrspl
Copy link
Contributor

kyrspl commented Aug 22, 2023

Here are EUI's combinations applied to this scenario. Option (3) is my preferred for this use case

(1)

background-color: ${euiTheme.colors.emptyShade};
color: ${euiTheme.colors.text};

Image

(2)

background-color: ${euiTheme.colors.lightestShade};
color: ${euiTheme.colors.text};

Image

(3)

background-color: ${euiTheme.colors.lightShade};
color: ${euiTheme.colors.title};

Image

(4)

background-color: ${euiTheme.colors.mediumShade};
color: ${euiTheme.colors.title};

Image

(5)

background-color: ${euiTheme.colors.darkestShade};
color: ${euiTheme.colors.ink};

Image

@ryankeairns
Copy link
Contributor Author

Thanks for putting these together. I also prefer number (3)

@andreadelrio any thoughts from your Discover point of view?

@kyrspl
Copy link
Contributor

kyrspl commented Aug 22, 2023

FYI - once we take the decision I can integrate the change with #7062

@andreadelrio
Copy link
Contributor

andreadelrio commented Aug 22, 2023

Thanks for putting these together. I also prefer number (3)

@andreadelrio any thoughts from your Discover point of view?

In Discover, we're getting rid of the background in the title altogether but as discussed with the rest of the EUI team we'll implement that change (as well as increasing font weight) only in the Kibana repo. Having said that I also prefer option # 3. Attaching screenshot of the plans for Discover for context.

image

@kyrspl
Copy link
Contributor

kyrspl commented Aug 23, 2023

Thanks for your comments. I've incorporated option (3) in #7062

Copy link

👋 Hi there - this issue hasn't had any activity in 6 months. If the EUI team has not explicitly expressed that this is something on our roadmap, it's unlikely that we'll pick this issue up. We would sincerely appreciate a PR/community contribution if this is something that matters to you! If not, and there is no further activity on this issue for another 6 months (i.e. it's stale for over a year), the issue will be auto-closed.

@mgadewoll mgadewoll self-assigned this Mar 25, 2024
@mgadewoll
Copy link
Contributor

This has already been fixed some time ago in this PR (see the exact change here)

🔗 See the current released EUI docs here.

Image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility low hanging fruit An issue, often a bug, that is lower effort and clearly ought to be fixed
Projects
None yet
Development

No branches or pull requests

5 participants