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

Jump to bottom

fix(toast): Toast should have API for overriding the icon's alt text #4159

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

fix(toast): Toast should have API for overriding the icon's alt text #4159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fd0b6a4
chore(action-menu): include action menu label in stories and READMEs …
audknu Mar 5, 2024
5fe626c
fix(Toast): adding label for icon
Mar 7, 2024
c4cdb8c
fix(toast): making label customizable on storybook
Mar 8, 2024
7f4b08e
fix(toast): update unit test
Mar 8, 2024
3051037
fix(toast): remove incorrect test
Mar 8, 2024
aef34dc
fix(toast): update unit tests and add default label
Mar 8, 2024
913d15e
Merge branch 'main' of https://github.com/adobe/spectrum-web-componen…
Mar 15, 2024
99bf2ff
fix(toast): updated type in test
Mar 15, 2024
1ddb7b5
Merge branch 'main' into yenugula/3406-fix
susmithayenugula Mar 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/icon/src/IconBase.ts
View file
Open in desktop
Expand Up @@ -26,7 +26,7 @@ export class IconBase extends SpectrumElement {
return [iconStyles];
}

@property()
@property({ type: String, reflect: true })
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of reflecting here?

public label = '';

@property({ reflect: true })
Expand All @@ -36,6 +36,7 @@ export class IconBase extends SpectrumElement {
if (changes.has('label')) {
if (this.label) {
this.removeAttribute('aria-hidden');
this.setAttribute('aria-label', this.label);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per the readme for icon:

aria-hidden is set to true by default for Icons. The label attribute suppresses this and adds the label text as the aria-label of the icon.

This was not implemented so fixed it in this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why we don't use aria-label from the component tag instead of introducing a new label property? I removed the custom label property and use the aria-label that can be specified at the component level

Copy link
Contributor

Choose a reason for hiding this comment

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

At the API level we generally use label because sometimes that element must be labeled at the host (which would be the same as using aria-label, but other times that label needs to be applied to something within the host, and forwarding aria-label is a tricky accessibility/timing cup game of dispair. Having a single entry point across as many elements as possible intends that it is less likely that a consumer makes the mistake of using the wrong one when switching back and forth between them over the course of developing a larger application.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great add. However, this whole updated method probably should be moved to Icon.ts instead. Some IconBase.ts extensions will pass the label data into an icon literal, which also manages aria-hidden, however these management would be beneficial to the <sp-icon> element as it accepts a slotted icon rather than owning icon DOM itself.

} else {
this.setAttribute('aria-hidden', 'true');
}
Expand Down
26 changes: 21 additions & 5 deletions packages/toast/src/Toast.ts
View file
Open in desktop
Expand Up @@ -116,28 +116,44 @@ export class Toast extends FocusVisiblePolyfillMixin(SpectrumElement) {
return this._variant;
}

@property({ type: String })
public set label(label: string) {
this._label = label;
this.setAttribute('label', label);
this.requestUpdate('label', label);
}

public get label(): string {
return this._label;
}
Comment on lines +119 to +128
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use a simple decorator here:

@property()
label = '';

And then avoid all the rest of the manual administration?

As far as naming, this isn't so much the label of the toast as much as the label of the icon. Should we have a more specific attribute/property name that represents that? Which ever way we go, it'll be good to have a JSDoc description as to how the value is leveraged within the element.


private _variant: ToastVariants = '';

private renderIcon(variant: string): TemplateResult {
private _label: string = '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure but seems like changing the default string to be an empty string as opposed to the existing Success, Error, and Information seems like it could break things?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, if the consumer does not define a label, should we fall back to the existing strings?

Suggested change
private _label: string = '';
<sp-icon-info label=${label === "" ? "Information" : label} class="type"></sp-icon-info>

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah falling back to the default makes sense to me!


private renderIcon(variant: string, label: string): TemplateResult {
switch (variant) {
case 'info':
return html`
<sp-icon-info
label="Information"
label=${label === '' ? 'Information' : label}
Copy link
Contributor

Choose a reason for hiding this comment

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

'' is falsy, so this can be simplified to label || 'Information'.

class="type"
></sp-icon-info>
`;
case 'negative':
case 'error': // deprecated
case 'warning': // deprecated
return html`
<sp-icon-alert label="Error" class="type"></sp-icon-alert>
<sp-icon-alert
label=${label === '' ? 'Error' : label}
class="type"
></sp-icon-alert>
`;
case 'positive':
case 'success': // deprecated
return html`
<sp-icon-checkmark-circle
label="Success"
label=${label === '' ? 'Success' : label}
class="type"
></sp-icon-checkmark-circle>
`;
Expand Down Expand Up @@ -205,7 +221,7 @@ export class Toast extends FocusVisiblePolyfillMixin(SpectrumElement) {

protected override render(): TemplateResult {
return html`
${this.renderIcon(this.variant)}
${this.renderIcon(this.variant, this.label)}
<div class="body" role="alert">
<div class="content">
<slot></slot>
Expand Down
19 changes: 17 additions & 2 deletions packages/toast/stories/toast.stories.ts
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ const toast = ({
variant = '',
open = true,
content = '',
label = '',
}): TemplateResult => html`
<sp-toast
variant=${variant as
Expand All @@ -33,6 +34,7 @@ const toast = ({
| 'error'
| 'warning'}
?open=${open}
label=${label}
>
${content}
<sp-button
Expand All @@ -52,6 +54,7 @@ export default {
args: {
content: 'This is a toast message.',
open: true,
label: 'testLabel',
},
argTypes: {
content: {
Expand All @@ -75,30 +78,42 @@ export default {
type: 'boolean',
},
},
label: {
name: 'label',
type: { name: 'string', required: false },
table: {
type: { summary: 'string' },
defaultValue: { summary: '' },
},
control: 'text',
},
},
};

interface Properties {
variant: '' | 'negative' | 'positive' | 'info' | 'error' | 'warning';
open: boolean;
content: string;
label: string;
onClose: (event: Event) => void;
}

export const Default = ({
variant,
open,
content,
label,
}: Properties): TemplateResult => {
return toast({ variant, open, content });
return toast({ variant, open, content, label });
};

const variantDemo = ({
variant,
open,
content,
label,
}: Properties): TemplateResult => {
return toast({ variant, open, content });
return toast({ variant, open, content, label });
};

export const Positive = (args: Properties): TemplateResult =>
Expand Down
71 changes: 69 additions & 2 deletions packages/toast/test/toast.test.ts
View file
Open in desktop
Expand Up @@ -56,15 +56,82 @@ describe('Toast', () => {
it(`loads - [variant="${variant}"]`, async () => {
const el = await fixture<Toast>(
html`
<sp-toast variant=${variant} open>
<sp-toast variant=${variant} open label="testLabel">
This toast is of the \`${variant}\` variant.
</sp-toast>
`
);
await elementUpdated(el);
await expect(el.getAttribute('label')).to.equal('testLabel');
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 couldn't check if the sp-icon's label in this test since sp-icon isn't exposed in sp-toast

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you try getting it through the shadowRoot? So:

el.shadowRoot.querySelector(
   sp-icon-checkmark-circle
) as HTMLElement;

I see that used in https://github.com/adobe/spectrum-web-components/blob/main/packages/tray/test/tray.test.ts#L105

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarahszhou Thank you, worked perfectly!

await expect(el).to.be.accessible();
});

it(`customized label for sp-icon for [variant="${variant}"]`, async () => {
const el = await fixture<Toast>(
html`
<sp-toast variant=${variant} open label="testLabel">
This toast is of the \`${variant}\` variant.
</sp-toast>
`
);

const getIconType = (): string => {
switch (variant) {
case 'info':
return 'sp-icon-info';
case 'negative':
case 'error': // deprecated
case 'warning': // deprecated
return 'sp-icon-alert';
case 'positive':
return 'sp-icon-checkmark-circle';
default:
return '';
}
};
Comment on lines +78 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be alleviated by the fact that the icon always has class="type"?


await elementUpdated(el);
const icon = el.shadowRoot.querySelector(
getIconType()
) as HTMLElement;
await expect(icon.getAttribute('label')).to.equal('testLabel');
});

await expect(el).to.be.accessible();
it(`default label for sp-icon for [variant="${variant}"]`, async () => {
const el = await fixture<Toast>(
html`
<sp-toast variant=${variant} open>
This toast is of the \`${variant}\` variant.
</sp-toast>
`
);

let iconType;
let defaultLabel;
switch (variant) {
case 'info':
iconType = 'sp-icon-info';
defaultLabel = 'Information';
break;
case 'negative':
case 'error': // deprecated
case 'warning': // deprecated
iconType = 'sp-icon-alert';
defaultLabel = 'Error';
break;
case 'positive':
iconType = 'sp-icon-checkmark-circle';
defaultLabel = 'Success';
break;
default:
iconType = '';
defaultLabel = '';
break;
}
Comment on lines +109 to +130
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of this can be simplified via class="type" being on the icon element. The rest of the data feels like it should be externalized some how, but I don't have a good suggestion on cleaning it up just now. Maybe an map of some sort outside of the test case?


await elementUpdated(el);
const icon = el.shadowRoot.querySelector(iconType) as HTMLElement;
await expect(icon.getAttribute('label')).to.equal(defaultLabel);
});
});
it('loads - timeout', async () => {
Expand Down