Skip to content
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

Switch to using d2l-link in object-property-item-link #5183

Merged
merged 2 commits into from
Nov 27, 2024
Merged

Switch to using d2l-link in object-property-item-link #5183

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df0089a
Switch to using d2l-link in object-property-item-link
ChrisLabattD2L Nov 27, 2024
a0e8393
Remove redundant aria-label
ChrisLabattD2L Nov 27, 2024
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
1 change: 1 addition & 0 deletions components/object-property-list/demo/object-property-list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ <h2>Object Property List: All item variants</h2>
<d2l-object-property-list-item text="Example item"></d2l-object-property-list-item>
<d2l-object-property-list-item text="Example item with icon" icon="tier1:grade"></d2l-object-property-list-item>
<d2l-object-property-list-item-link text="Example link" href="https://www.d2l.com/"></d2l-object-property-list-item-link>
<d2l-object-property-list-item-link target="_blank" text="Example new tab link" href="https://www.d2l.com/"></d2l-object-property-list-item-link>
<d2l-object-property-list-item-link text="Example link with icon" href="https://www.d2l.com/" icon="tier1:alert"></d2l-object-property-list-item-link>
</d2l-object-property-list>
</template>
Expand Down
20 changes: 6 additions & 14 deletions components/object-property-list/object-property-list-item-link.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import '../link/link.js';
import { FocusMixin } from '../../mixins/focus/focus-mixin.js';
import { html } from 'lit';
import { ifDefined } from 'lit/directives/if-defined.js';
import { linkStyles } from '../link/link.js';
import { ObjectPropertyListItem } from './object-property-list-item.js';

/**
Expand Down Expand Up @@ -29,29 +29,21 @@ class ObjectPropertyListItemLink extends FocusMixin(ObjectPropertyListItem) {
};
}

static get styles() {
return [
...super.styles,
linkStyles
];
}

static get focusElementSelector() {
return '.d2l-link';
return 'd2l-link';
}

render() {
return html`
${this._renderIcon()}
${!this.skeleton ? html`
<a
<d2l-link
aria-label="${this.text}"
Copy link
Member

Choose a reason for hiding this comment

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

The aria-label shouldn't be needed? I'd expect its label to come from its internals via renderText().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd mostly added it since it was a property of d2l-link, but you're right that it should just be handled within renderText. Though it does beg the question, if this component should have some property to provide more context to screen reader users like how aria-label is currently doing for d2l-link. I'm good with just removing it for now though!

Copy link
Member

Choose a reason for hiding this comment

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

Setting aria-label in this way actually replaces the contents, so there wouldn't be more context in the same way aria-description might provide.

Having aria-label on <d2l-link> as a passthrough to its underlying <a>'s aria-label was a mistake we made initially. All the aria-* attribute are obviously native attributes and aXe has actually started complaining recently when we put them on custom elements.

What we should have done (and have correct in newer things) is to use a combination of text (or label) and text-hidden/label-hidden to flip that to the aria-label of the native element. And then add something like a description attribute to go into the aria-description.

?download="${this.download}"
class="d2l-link"
href="${ifDefined(this.href)}"
target="${ifDefined(this.target)}"
>
target="${ifDefined(this.target)}">
${this._renderText()}
</a>
</d2l-link>
` : this._renderText()}
${this._renderSeparator()}
`;
Expand Down
Loading