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

Docs: improve progress bar labels markup and explanations for accessibility #39364

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Nov 3, 2023

Description

Usage of color and background helper classes

In the current Progress > Backgrounds documentation, we say the following:

Alternatively, you can use the new combined color and background helper classes.”

In the same spirit as what we've done in #39214, this PR suggests always using .text-bg-{color} when a label is used within the progress component. If one changes the colors between the light and the dark mode (contrary to Bootstrap), you'd like the text color to adapt automatically in light and dark mode to have sufficient contrast. And it can't be done with .text-dark or other utilities.

The idea here is to:

  • keep the examples without labels with .bg-{color}
  • change the label examples to use .text-bg-{color}
  • modify the documentation explanation to always suggest the use of .text-bg-{color}

Note: we talked about it internally and @mdo seemed to agree with this option

Long labels

The second part of the current Progress > Labels mentions something to handle the long labels. It's already mentioned that “Be aware though that currently this approach does not take into account color modes.”

However, I don't think there's anything we can provide that will make it work all the time.

Right now, we use .text-dark which obviously doesn’t work in dark mode as mentioned.
Based on my previous proposal, we should rather use .text-bg-primary which works well in dark mode and half-works in light mode (only on the blue color):

Screenshot 2023-11-03 at 08 18 06

The issue here is that we will never find anything that can work for both colors with the actual color palette. It can only work if the color palette is compatible and that .progress and .progress-bar have the same tint. Said differently, that they are both using light colors or dark colors so that the text color has sufficient contrast.

Since we don't know how the color palettes of projects will be built, I suggest that we don't show this example anymore in our documentation but still explain that it's possible, but with a big warning regarding accessibility and contrast ratio, and that we rather recommend most of the time to display the label outside the progress bar.

Note: we talked about it internally and @mdo seemed to agree with this option

Friendly ping for @mdo and @patrickhlauke for wording and double-checking the explanation. We might want to show how to display the label outside of the progress bar and keep it accessible.

Please also note that I haven't changed the Cheatsheet example since the values fit within the progress bar and don't have any contrast ratio issues.

Target version

IMO this is non-breaking for users so it could be embedded directly into the v5.3.x. Thoughts?

Type of changes

  • Documentation (non-breaking change)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • (N/A) I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Note that by default, the content inside the `.progress-bar` is controlled with `overflow: hidden`, so it doesn't bleed out of the bar. If your progress bar is shorter than its label, the content will be capped and may become unreadable. To change this behavior, you can use `.overflow-visible` from the [overflow utilities]({{< docsref "/utilities/overflow" >}}).

{{< callout warning >}}
**Accessibility warning:** Long labels may not be fully accessible with this method. As it relies on the text color having the right contrast ratio with both the `.progress` and `.progress-bar` background colors, your color palette could be incompatible with this approach.
Copy link
Member

Choose a reason for hiding this comment

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

the colour problem applies to short labels inside progress bars as well though, so i wouldn't make this callout specific to long labels. move it further down where you talk about setting text colours etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

the colour problem applies to short labels inside progress bars as well though

I'm not sure to understand precisely this point.

My approach was the following:

  • .text-bg-* is taking care of the contrast issues (and if overridden, should be done by the users to ensure the right contrast).
  • When folks don't want to use these utilities, the following sentence is supposed to cover this use case: "... make sure to also set an appropriate text color, so the labels remain readable and have sufficient contrast".

So the remaining case would be the label not fitting in the really small space -> use overflow-visible but be careful (the yellow accessibility callout) because you probably never find a color contrast working for both the colored progress bar, the gray area behind, and the text over these 2 surfaces.

By moving the yellow callout down and making it non-specific to long labels, I have the impression that a use case wouldn't be explained/understood.

Do you have an idea/a wording to tackle everything in one callout moved further down? Don't hesitate to push a commit on top of mine. Would be great if there was a simplified way to explain that, my wording capacities are limited in English ^^

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure to understand precisely this point.

unless i'm missing something, regardless of length of the label, it will always have to contrast against both the progress bar itself and the progress background. it's just that it's likely that for "long" labels, the problem is likely more obvious?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about the first two use cases in https://codepen.io/julien-deramond/pen/QWYxwXV where the overflow is hidden or when the value is static and the developers know whether the label can always fit within the progress bar and will never overlap the progress background.
Maybe it's a bad practice but I saw that on websites.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, getting back to this after a long break... but my point is even in those examples you mention, look at the first one...it doesn't quite fit into the blue bar either.

screenshot of the codepen, with the first '1%' text circled, as it also doesn't fully sit over the blue progress bar

so my point is: the callout applies in all situations - you'll likely never find a colour for the text that contrasts sufficiently against both the bar and the bar background, and particularly when the progress bar is actually used to show dynamic progress, you won't generally be able to guarantee that text fits in perfectly under all conditions. that's why i'd say the callout should be generalised, as it's not just a problem with dark mode etc

Copy link
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

see comment about moving the callout

@mdo
Copy link
Member

mdo commented Mar 18, 2024

Adding the v5.4.0 project here too @julien-deramond in case we want to save this for a larger release as opposed to a patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review in progress
Status: Needs review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants