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

Breadcrumbs text and default links fail contrast ratio #32053

Closed
patrickhlauke opened this issue Nov 2, 2020 · 5 comments · Fixed by #32249
Closed

Breadcrumbs text and default links fail contrast ratio #32053

patrickhlauke opened this issue Nov 2, 2020 · 5 comments · Fixed by #32249

Comments

@patrickhlauke
Copy link
Member

patrickhlauke commented Nov 2, 2020

https://v5.getbootstrap.com/docs/5.0/components/breadcrumb/

contrast-bootstrap-breadcrumbs

The active text/separator for breadcrumbs uses #6c757d on #e9ecef with a contrast ratio of 4:1. The default links in breadcrumbs use #0d6efd on #e9ecef with a contrast ratio of 3.8:1.

bumping the text/separator from $gray-600 to $gray-700 (#495057) shoots up the contrat to 6.9:1. For the links, however, there's very little wiggle room ... not sure how/if that can be tackled - #0d6efd has a ratio of 4.5:1 only against pure white, so even lightening the breadcrumbs background a bit won't help

@patrickhlauke
Copy link
Member Author

patrickhlauke commented Nov 2, 2020

maybe dropping the background completely on breadcrumbs and going for a thin gray border instead?

contrast-bootstrap-breadcrumbs-amended

matgargano added a commit to matgargano/bootstrap that referenced this issue Nov 6, 2020
@ffoodd
Copy link
Member

ffoodd commented Nov 6, 2020

I think I suggested that somewhere. Honestly I'd drop both background and borders, to simplify theming.

@ffoodd
Copy link
Member

ffoodd commented Nov 20, 2020

To complement my previous comment, adding background, border or radii is really, really easy thanks to our utilities. So apart from spacings, I'd say this component should definetely be simpler — and as an argue, the vast majority of breadcrumlbs out on the web are much simpler than our current one.

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta1 via automation Nov 21, 2020
@mdo
Copy link
Member

mdo commented Nov 23, 2020

Let's drop the background, border, and padding then. The main use of a breadcrumb component then is purely to use add a separator between nested items. I can dig that. I'm also thinking we use a local CSS var for the separator: https://codepen.io/emdeoh/pen/KKMOoGx. Note that the first example I tried using a data attribute, but that doesn't work as it doesn't seem to be exposed to the children elements.

@ffoodd
Copy link
Member

ffoodd commented Nov 25, 2020

Regarding the CSS custom property, we might do as in another place, using an unassigned custom property and our Sass variables as a fallback:

.breadcrumb-item + .breadcrumb-item::before {
  content: var(--bs-separator, #{$breadcrumb-separator)};
}

v5.0.0-beta1 automation moved this from Inbox to Done Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5.0.0-beta1
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants