Add role to empty links #810
Conversation
|
@kmeleta Also check |
@svinkle the section link for multicolumn appeared to already have the proper attributes. I'll make sure the new column specific links we're adding in the other PR have the same as well. If you think of anywhere else I might check though definitely let me know. |
| @@ -93,10 +93,10 @@ | |||
| {%- when 'buttons' -%} | |||
| <div class="banner__buttons{% if block.settings.button_label_1 != blank and block.settings.button_label_2 != blank %} banner__buttons--multiple{% endif %}" {{ block.shopify_attributes }}> | |||
| {%- if block.settings.button_label_1 != blank -%} | |||
| <a{% if block.settings.button_link_1 != blank %} href="{{ block.settings.button_link_1 }}"{% endif %} class="button{% if block.settings.button_style_secondary_1 %} button--secondary{% else %} button--primary{% endif %}"{% if block.settings.button_link_1 == blank %} aria-disabled="true"{% endif %}>{{ block.settings.button_label_1 | escape }}</a> | |||
| <a{% if block.settings.button_link_1 == blank %} role="link" aria-disabled="true"{% else %} href="{{ block.settings.button_link_1 }}"{% endif %} class="button{% if block.settings.button_style_secondary_1 %} button--secondary{% else %} button--primary{% endif %}">{{ block.settings.button_label_1 | escape }}</a> | |||
There was a problem hiding this comment.
Just a pattern we can apply here.
Since there's always going to be something being outputted by this condition, we can move the whitespace before the {% if %}.
| <a{% if block.settings.button_link_1 == blank %} role="link" aria-disabled="true"{% else %} href="{{ block.settings.button_link_1 }}"{% endif %} class="button{% if block.settings.button_style_secondary_1 %} button--secondary{% else %} button--primary{% endif %}">{{ block.settings.button_label_1 | escape }}</a> | |
| <a {% if block.settings.button_link_1 == blank %}role="link" aria-disabled="true"{% else %}href="{{ block.settings.button_link_1 }}"{% endif %} class="button{% if block.settings.button_style_secondary_1 %} button--secondary{% else %} button--primary{% endif %}">{{ block.settings.button_label_1 | escape }}</a> |
This would apply to every change in this PR.
There was a problem hiding this comment.
Ah yeah good observation, I'm not sure I feel too strongly about it though. I think I'd have to either move the class attr in front of the others or put it on a new line or else risk a double space. So I don't feel we're gaining much from it.
There was a problem hiding this comment.
Looks good!
I think one other place it could be added is in the collection-list.liquid for:
<a{% if block.settings.collection != blank and block.settings.collection.all_products_count > 0 %} href="{{ block.settings.collection.url }}"{% endif %}
class="card animate-arrow link{% if block.settings.collection.featured_image != blank %} card--media{% else %}{% if section.settings.image_ratio != 'adapt' %} card--stretch{% endif %}{% endif %}{% unless section.settings.image_padding %} card--light-border{% endunless %}"
>
* Add role to empty links * Also collection list
Why are these changes introduced?
Fixes #387
What approach did you take?
Disabled/empty links should have
aria-disabled="true"androle="link"instead of ahref. I foundimage-banner,image-with-text, andrich-textneeded the role. I didn't find any other areas where we render links with no href.Demo links
Checklist