-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[4.0] [a11y] Contrast of Drop down chevron #28680
Conversation
See #28463 for hint to solve the issue. |
color: theme-color("success"); | ||
background-color: theme-color("success"); | ||
border-color: theme-color("success"); | ||
|
||
[dir=rtl] & { | ||
background: $custom-select-background-light-rtl; | ||
color: theme-color("success"); |
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.
No need to redefine these color properties in RTL (color, bg and border)
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.
Thank you @ciar4n, but I need color, bg and border for rtl, as it is set for the variable
$custom-select-background-light-rtl
and I have to override it.
I could set the properties in the variable $custom-select-background-light-rtl
differently. But in my opinion, this would make a bigger mess because it would be very different from $custom-select-background-rtl
.
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.
Actually I see why you might need background-color
as you are re-setting the color to transparent with background: $custom-select-background-light-rtl;
. The border and color should be fine without but maybe I'm missing something.
color: theme-color("danger"); | ||
background-color: theme-color("danger"); | ||
border-color: theme-color("danger"); | ||
|
||
[dir=rtl] & { | ||
background: $custom-select-background-light-rtl; | ||
color: theme-color("danger"); |
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.
No need to redefine these color properties in RTL (color, bg and border)
I dont understand why there is a new image. I though the whole point of using an svg is that you can change its colour using css eg |
Because it is a background image, it is not part of the DOM and therefore, to my knowledge, can't be changed via css. The svg will need to be added inline. Eg. https://chriswrightdesign.github.io/customforms/select-box-svg/ |
Thanks @ciar4n I didn't know about the background image but that makes total sense |
In a perfect world, they should be all the same. My preference would be inline svg. If you look deep enough, you will even find some css chevrons in the mix. |
How would you do it exactly? If we enter the svg directly in the markup, we can not reuse things, right?
Then we can use it like this
|
What is the advantage of using svg vs icons? As I see it, it rather makes things more complex than necessary. |
Size... with SVG you just load what you need rather than an entire library (css & font). Also SVG allows us to drop in custom fonts rather than been restricted to a set library. Regardless I think Font Awesome is fine for the backend as it is just one template that will likely be ever used. The frontend is a slightly different matter. For this PR I think ideally we change the choices icon to Font Awesome, bringing it inline with the majority of the admin. If that is not possible, then this PR as it is fixes the issue. |
I have not tested this item. This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28680. |
@adj9 Thanks for your test. May I ask you: How did you test? Did you use the patch tester? |
@ciar4n @infograf768 Thanks for your opinion. I'll look at that tomorrow. |
Please test PR #28755 |
Draft for Issue #28673.
Edit: To test this draft, it is necessary to use the
npm ci
command, as SCSS files need to be compiled.Summary of Changes
I made it possible, that the drop down chevron on
success
anddanger
drop downs is white (not black).For the red danger background the Contrast Ratio is even worse. It is 3.71.
When editing a module, the white arrow also fits better with the calendar symbol at start publishing.
Next you see the white chevron on a drop down with the green
success
background while editing a plugin. Note: The chevrons on the drop downs with the gray background are still black.I made this as a draft, because I am not sure, if I understand the way we want to handle multiple background elements. Perhaps someone like to teach me?