Add alt text support to decorateIcon #280
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
|
|
I don't know how to provide a valid PSI after URL. I can push a branch to the upstream repo, however I do not have rights to the Content source to make the change there to reflect tests. Here's the PSI check of my demo page |
|
i don't think that the decorate method (as it is called very early) should have a dependency on placeholders as that may add (variable and possibly considerable) weight to before LCP. i think icons and alt texts and title / description inside the SVGs are an interesting quandary where i am not totally sure what's the correct solution, but i think that they don't need to be added before the icons are loaded (via lazy or intersection observer). i think @ramboz has a PR (i forgot where) to enable spriting (and title / alt handling). |
All sprites were removed during the renaming process. to Even if the SVG has title/alt text inside the file itself, i'm not sure that solves this issue - Since the @amol-anand indicated this might be a conern. I will remove the placeholders, but there should always be a default - it can just be the |
|
Right, I submitted the old spriting to the block party, but haven't heard back from the team yet. You can find the PR here: ramboz#12 |
scripts/aem.js
Outdated
| @@ -395,16 +395,18 @@ function decorateButtons(element) { | |||
|
|
|||
| /** | |||
| * Add <img> for icon, prefixed with codeBasePath and optional prefix. | |||
| * @param {span} [element] span element with icon classes | |||
| * @param {Element} [span] span element with icon classes | |||
| * @param {string} [prefix] prefix to be added to icon the src | |||
| */ | |||
| function decorateIcon(span, prefix = '') { | |||
There was a problem hiding this comment.
I think we need to make this configurable. It only makes sense to add the alt attribute if we have an actual valuable description to expose via screen readers. If the icons are only used for decoration purposes, then it's usually better to just annotate them with role="presentation" alt=" ".
I'd rather propose something like:
| function decorateIcon(span, prefix = '') { | |
| function decorateIcon(span, prefix = '', alt = '') { |
There was a problem hiding this comment.
I did commit this, but my only concern with this is - there's no value coming from the decorateIcons wrapping iteration. This function is not exported, so where should the caller source this value?
There was a problem hiding this comment.
You'd have to export the single decorateIcon as well and then explicitly instrument in the blocks that require proper alt text
Co-authored-by: Julien Ramboz <ramboz@users.noreply.github.com>
Co-authored-by: Julien Ramboz <ramboz@users.noreply.github.com>
|
@bstopp i think this PR needs to be moved to |
|
Didn't realize that. i'll move it this weekend. |
@bstopp, please take a look here when you get a chance: adobe/aem-lib#26 |
|
Closing as moved to aem-lib |
Default the Alt Text to the iconName as referenced in markdown.
Also support i18n lookup based on
icon-<icon-name>key.Fix #279
Test URLs: