Skip to content

OEL-2127: Add visually-hidden element when there is no label for close button in offcanvas #684

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from
Open

OEL-2127: Add visually-hidden element when there is no label for close button in offcanvas #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ddb22a0
OEL-2127: When there is no aria-label attribute for close button, we …
vengador Jul 14, 2025
395b550
OEL-2127: Fix broken test due to changes in markup for offcanvas when…
vengador Jul 14, 2025
c8071db
OEL-2127: Increasing timeout for some tests failing due to default ti…
vengador Jul 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/components/bcl-offcanvas/offcanvas.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
{% set _with_backdrop = with_backdrop ?? true %}
{% set _with_close = with_close ?? true %}
{% set _close_aria_label = close_aria_label|default('') %}
{% set _close_content = '' %}
Copy link
Collaborator

@AaronGilMartinez AaronGilMartinez Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment about code structure, we can put all parameters that can be set at the top and then variables used by the template.

Another option is to add the parameter, but then the pattern should add it too.

So I'd set by default this after line 77 or so:

{% set _close_content %}
        <span class="label visually-hidden">Close</span>
 {% endset %}

{% set _extra_classes_body = extra_classes_body|default('') %}
{% set _extra_classes_header = extra_classes_header|default('') %}
{% set _extra_classes_close = extra_classes_close|default('') %}
Expand Down Expand Up @@ -89,18 +90,28 @@
} only %}
{% endif %}
{% if _with_close %}
{% set button_attributes = create_attribute()
{% set _close_attributes = create_attribute()
.addClass(['btn-close', 'text-reset'])
.setAttribute('data-bs-dismiss', 'offcanvas')
.setAttribute('data-bs-target', '#' ~ _id)
.setAttribute('aria-label', _close_aria_label)
%}

{% if _extra_classes_close is not empty %}
{% set button_attributes = button_attributes.addClass(_extra_classes_close) %}
{% set _close_attributes = _close_attributes.addClass(_extra_classes_close) %}
{% endif %}

{% if _close_aria_label %}
{% set _close_attributes = _close_attributes.setAttribute('aria-label', _close_aria_label) %}
{% else %}
Comment on lines +103 to +105
Copy link
Collaborator

@AaronGilMartinez AaronGilMartinez Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here we avoid the else:

Suggested change
{% if _close_aria_label %}
{% set _close_attributes = _close_attributes.setAttribute('aria-label', _close_aria_label) %}
{% else %}
{% if _close_aria_label %}
{% set _close_attributes = _close_attributes.setAttribute('aria-label', _close_aria_label) %}
{% set _close_content = '' %}
{% endif %}

{% set _close_content %}
<span class="label visually-hidden">{{ "Close" }}</span>
Copy link
Collaborator

@AaronGilMartinez AaronGilMartinez Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can add the translation filter here and if we should, this texts may not get translated.

{% endset %}
{% endif %}

{%- include '@oe-bcl/bcl-button/button.html.twig' with {
attributes: button_attributes,
clean_class: true
'label': _close_content,
'attributes': _close_attributes,
'clean_class': true
} only -%}
{% endif %}
</div>
Expand Down
9 changes: 7 additions & 2 deletions src/compositions/bcl-group/__snapshots__/group.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2808,12 +2808,17 @@ exports[`OE - Group listing Content renders correctly 1`] = `
Filter options
</h2>
<button
aria-label=""
class="btn-close text-reset d-lg-none"
data-bs-dismiss="offcanvas"
data-bs-target="#bcl-offcanvas"
type="button"
/>
>
<span
class="label visually-hidden"
>
Close
</span>
</button>
</div>
<div
class="offcanvas-body p-lg-0"
Expand Down
2 changes: 1 addition & 1 deletion src/compositions/bcl-page/page.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ describe("OE - Page", () => {
expect(
await axe(renderTwigFileAsHtml(template, demoData)),
).toHaveNoViolations();
});
}, 10000);
});
2 changes: 1 addition & 1 deletion src/compositions/bcl-search/search.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ describe("OE - Search", () => {
expect(
await axe(renderTwigFileAsHtml(template, dataListing)),
).toHaveNoViolations();
});
}, 10000);
});
Loading