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

fix: remove overflow:hidden from progressbar (issue: 37821) #37849

Closed
wants to merge 3 commits into from
Closed

fix: remove overflow:hidden from progressbar (issue: 37821) #37849

wants to merge 3 commits into from

Conversation

alenap93
Copy link

@alenap93 alenap93 commented Jan 10, 2023

Description

This PR reverts changes made with #29629 for IE (with v5 we dropped the support). If percentage of progressbar is low (ie: 0%), the progressbar do not show labels (or cut it), therefore i have removed the overflow:hidden.

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

Related issues

Closes #37821

@julien-deramond
Copy link
Member

/cc @patrickhlauke for info since you've worked recently on this topic and maybe there's something regarding a11y

@patrickhlauke
Copy link
Member

patrickhlauke commented Jan 10, 2023

not quite sure about this. without the overflow rule, even in chrome etc, the text ends up being there (contrary to what #29622 says ... it just happens that the 0% bar was getting overlapped/covered by the following progress bars)

so, without overflow, you end up with this sort of thing (on a single progress bar) (using https://deploy-preview-37849--twbs-bootstrap.netlify.app/docs/5.3/components/progress/#labels and manually hacking away at it to set the progress to 0)

screenshot showing a 0% progress bar from the docs, with the "0%" text showing very faintly in white over light gray of the empty progress bar track

not sure if that's of any use to anybody without additional work on making the label text visible?

@patrickhlauke
Copy link
Member

wondering what the actual use case is you're trying to support by removing the overflow:hidden ... or if it's just intended as a "we probably don't need this CSS rule, so can remove it"?

@alenap93
Copy link
Author

alenap93 commented Jan 10, 2023

@patrickhlauke I know that with this configuration we violate some rules of accessibility contrast, but the intent is to remove the overflow and delegate the right color to the developer based on the use case...

@patrickhlauke
Copy link
Member

patrickhlauke commented Jan 10, 2023

but my point is: if we delegate making it look/work properly to developers anyway (for them to set a proper text color to make it actually usable by anybody), we can also delegate to developers that they remove the overflow:hidden, no?

as in, by default, the bootstrap progress bar doesn't look half-arsed and broken when the bar is shorter than the label that they try to cram in

@julien-deramond
Copy link
Member

Instead of modifying the default behavior, we could add something into the "Labels" section in the documentation to explain that by default it's like this and that the behavior can be overriden by the end-user if needed without breaking accessibility (depending on the use case)

@patrickhlauke
Copy link
Member

also, to be clear: i'm not coming at this necessarily from an accessibility point of view, but just a general "this looks objectively broken otherwise" point of view

@alenap93
Copy link
Author

Just for completeness, the actual behaviour with 1% (the % is cut off):

screen

@mdo
Copy link
Member

mdo commented Jan 11, 2023

It's also important to note we have to test against two versions of the progress bar, since we implemented a new one in v5.3.0. Just calling out so we confirm both situations.

@patrickhlauke
Copy link
Member

two versions of the progress bar

the only difference structurally comes for the multiple progress bars, which now use a tweaked overall container, but this won't affect the presentation of the label inside each progress bar itself.

@patrickhlauke
Copy link
Member

I would suggest this alternative: keep the default as is, but document how authors that do explicitly want to opt into the "text overflows the progress bar" can achieve it #37921

@mdo
Copy link
Member

mdo commented Jan 21, 2023

Closing for suggested PR.

@mdo mdo closed this Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Progress bar text doesnt handle the 0% case
4 participants