-
Notifications
You must be signed in to change notification settings - Fork 56
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
CI: implement htmlcs
runner for pa11y-ci
#1677
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for boosted ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
3f17e6c
to
812153e
Compare
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
SonarCloud Quality Gate failed. 0 Bugs No Coverage information |
` • This element has insufficient contrast at this conformance level. Expected a contrast ratio of at least 4.5:1, but text in this element has a contrast ratio of 3.66:1. Recommendation: change text colour to #757575. (html > body > div:nth-child(6) > main > div:nth-child(3) > div:nth-child(25) > div:nth-child(1) > nav > ul > li:nth-child(1) > a) <a class="page-link">Previous</a>`
Here is a tracker for all the hidden elements.
What was removed and need to be backported to Bs maybe:
Things that should be discussed before merging:
|
@@ -521,7 +521,7 @@ Responsive tables make use of `overflow-y: hidden`, which clips off any content | |||
Across every breakpoint, use `.table-responsive` for horizontally scrolling tables. | |||
|
|||
<div class="bd-example"> | |||
<div class="table-responsive"> | |||
<div class="table-responsive" tabindex="0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about adding tabindex="0"
everywhere.
It seems not yet ok on our side (#1711) and on Boostrap's side (twbs/bootstrap#38971).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll remove this rule and hide some more elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thinks it's a safer option for now.
I just saw that the linked PR on Bootstrap side has been validated. We may now consider to follow their footsteps.
@@ -32,10 +32,10 @@ | |||
<small class="font-monospace text-body-secondary text-uppercase">{{- $lang -}}</small> | |||
<div class="d-flex ms-auto"> | |||
<button type="button" class="btn-edit text-nowrap"{{ with $stackblitz_add_js }} data-sb-js-snippet="{{ $stackblitz_add_js }}"{{ end }} title="Try it on StackBlitz"> | |||
<svg class="bi" aria-hidden="true"><use xlink:href="#lightning-charge-fill"/></svg> | |||
<svg class="bi" aria-hidden="true" focusable="false"><use xlink:href="#lightning-charge-fill"/></svg> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was needed for IE but we are no longer supporting IE so we may not need it anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it for the consistency of the doc + I think it's fine having it atm since Bootstrap have it too. Feel free to open a PR on Bootstrap side if you want to remove all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And what about adding focusable="false"
to bootstrap directly? 🤔
https://github.com/twbs/bootstrap/blob/main/site/layouts/shortcodes/example.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll propose a PR on this subject on Bs directly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I will resolve this comment once this change will be validated by their core team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some points to clarify and it will be ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remaining subjects:
- adding
focusable="false"
to svg inshortcodes/example.html
or not depending if it's possible to do directly on Bootstrap's side or not. - reevaluate the need to add
tabindex=0
since Bootstrap's core team reach an agreement on this subject and are ready to merge.
Related issues
#938
Description
Add a runner to
pa11y-ci
since it could detect some errors that aXe doesn't detect.The remaining errors may be fixed from twbs/bootstrap#38219.
Motivation & Context
Have a better ci. It's not much work compared to what it can detect and avoid. Seems to have a better algorithm to detect color issues. Following #938 to ensure that it won't break.
According to a11y expert, it's better to have as much test as possible since the algorithm are different and can be a bit more accurate to some points. Furthermore, it doesn't add much work to make it work.
Types of change
Live preview
https://deploy-preview-1677--boosted.netlify.app/docs/examples/cards/