Change inform-icon to fa-info-circle #18001
Conversation
|
(Standard links)
|
|
I'm good with the idea behind this! |
|
Me too. |
|
This is great. My only concern is that you are adding "Info" for all of the icons. The result is that a screen reader would say, "Info there are no records selected," "Info warning: delete will remove this permission," and so forth. The "Info" might be more of a distraction than a help. Meanwhile, I'm not sure if it needs to be a hover title for the icon either.
It's perfectly acceptable for an icon to have no title or screen reader text if it's just there for decoration. I'd suggest dropping the "Info" altogether.
Alternatively, if you think it's important to have, you might set it off with brackets or a colon: something so that a screen reader could understand it's distinct from the text that follows.
|
|
@agh1 That sounds fine - so |
The I think the text is semantically important and the icons aren't purely decorative. |
|
@mattwire yes, that will work fine, technically speaking.
@mikeymjco it's kind of the opposite: the `sr-only` class is a way to make the contents literally in-visible while not using a visibility or display CSS rule that a screen reader will pick up on. (Since those are used functionally often, a screen reader figures that hidden things should be hidden from everyone.)
The class name is a common convention but nothing special. A screen reader will treat `<span class="sr-only">` no different than `<span class="foo">`.
While I disagree with @mikeymjco about the semantic importance of the icon, I think that is a valid opinion. If the icon should have alternative text read by a screen reader, though, it should be set off somehow, like with brackets or a colon. Otherwise, the screen reader will just barrel through that and the text that follows.
|
|
Jenkins retest this please. |
|
@mattwire I don't disagree with the ok-without-test - but I try not to add that label to my own PRs because I believe that the label represents the assessment of another merger & we can't self-assess that |
|
@agh1 @MikeyMJCO I think if the 2 of you reach agreement that this PR is mergeable then I would be happy to merge it |
|
@agh1 @MikeyMJCO I've updated to remove the "Info" text. I only added it because that's how existing examples seemed to have it. But for this icon I agree it doesn't bring anything useful. If the screenreader reads the text then it's already got your attention! |
|
Docs PR: civicrm/civicrm-dev-docs#839 |
|
@agh1 - is this good to merge from your pov? |
|
Yep it's good by me @eileenmcnaughton
|
Overview
Replace
<div class=icon inform-icon>with fontawesome fa-icon-circle using icon helper. If we're ok with this I'll add a mapping to the docs here: https://docs.civicrm.org/dev/en/latest/framework/ui/#older-icon-systemBefore
Using legacy icon
eg.
After
Using standard function and current icon.
eg.
Technical Details
Per UI discussions and call. This is changed in all templates using find/replace.
Comments
@colemanw @demeritcowboy @agh @vingle @totten This is an example of a find/replace (similar to and built on the work that @agh1 did) to update some of our UI in one go.