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: Progress fix + proposal #38014

Merged
merged 3 commits into from
Feb 14, 2023
Merged

Conversation

louismaximepiton
Copy link
Member

Description

Tweak a bit the wording and remove some space.
Proposal to use aria-valuemax better inside multiple bars.

Motivation & Context

Better logic for documentation.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

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
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

NA

<div class="progress-bar bg-success"></div>
</div>
<div class="progress" role="progressbar" aria-label="Segment three" aria-valuenow="20" aria-valuemin="0" aria-valuemax="100" style="width: 20%">
<div class="progress" role="progressbar" aria-label="Segment three" aria-valuenow="20" aria-valuemin="0" aria-valuemax="30" style="width: 20%">
Copy link
Member Author

Choose a reason for hiding this comment

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

Those values could also be 15, 30, 55 or whatever but the sum should be 100 since the aria-valuenow equals the width percentage.

Copy link
Member

Choose a reason for hiding this comment

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

reminded of this discussion from the past #32973 (comment)

tl;dr: there's no real concept of interdependent progress bars that together form part of a whole in ARIA, and it depends really how an author is using these (either as literally a visual smushing together of various progress bars that go from 0-100%, or segments of something that totals 100% ... both of which I could envisage as fine in certain circumstances)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah I haven't made enough searchs to find this! Thanks a lot to pin this PR. I better understand the context and the actual implementation, reverting this for sure.

@@ -15,7 +15,7 @@ toc: true
Progress components are built with two HTML elements, some CSS to set the width, and a few attributes. We don't use [the HTML5 `<progress>` element](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/progress), ensuring you can stack progress bars, animate them, and place text labels over them.

- We use the `.progress` as a wrapper to indicate the max value of the progress bar.
- The `.progress` wrapper also requires a `role="progress"` and `aria` attributes to make it accessible, including an accessible name (using `aria-label`, `aria-labelledby`, or similar).
Copy link
Member

Choose a reason for hiding this comment

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

oops, good spot

@patrickhlauke
Copy link
Member

personally, i'm ok with all changes except for the valuemax changes (unless we expand this much further to explain the two possible ways to look at these stacked progress bars, e.g. three separate bars that go 0-100% but that have been visually smushed together, OR separate segments that form part of a whole)

@mdo mdo merged commit 7bffd6e into twbs:main Feb 14, 2023
@patrickhlauke
Copy link
Member

patrickhlauke commented Feb 14, 2023

party time 🥳

@louismaximepiton louismaximepiton deleted the main-lmp-progress-fix branch February 14, 2023 10:58
@mahilanmjd mahilanmjd mentioned this pull request Apr 16, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants