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 5 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
21 changes: 18 additions & 3 deletions packages/card/README.md
View file
Open in desktop
Expand Up @@ -107,7 +107,12 @@ Cards can be supplied an `actions` via a names slot.
alt="Demo Image"
/>
<div slot="footer">Footer</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand Down Expand Up @@ -181,7 +186,12 @@ Quiet cards will also accept `actions` via a named slot.
<sp-card variant="quiet" heading="Card Heading" subheading="JPG Photo">
<img alt="" slot="preview" src="https://picsum.photos/200/300" />
<div slot="description">10/15/18</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand Down Expand Up @@ -262,7 +272,12 @@ Or a `quiet` card:
>
<img src="https://picsum.photos/110" alt="Demo Image" slot="preview" />
<div slot="footer">Footer</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand Down
35 changes: 30 additions & 5 deletions packages/card/stories/card.stories.ts
View file
Open in desktop
Expand Up @@ -85,7 +85,12 @@ export const href = (args: StoryArgs): TemplateResult => {
Footer with a
<sp-link href="https://google.com">link to Google</sp-link>
</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand All @@ -112,7 +117,12 @@ export const actions = (args: StoryArgs): TemplateResult => {
>
<img slot="cover-photo" src=${portrait} alt="Demo Graphic" />
<div slot="footer">Footer</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand Down Expand Up @@ -222,7 +232,12 @@ export const quietActions = (args: StoryArgs): TemplateResult => {
>
<img src=${portrait} alt="Demo Graphic" slot="preview" />
<div slot="description">10/15/18</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand Down Expand Up @@ -289,7 +304,12 @@ export const smallQuiet = (args: StoryArgs): TemplateResult => {
>
<img src=${portrait} alt="Demo Graphic" slot="preview" />
<div slot="footer">Footer</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand Down Expand Up @@ -331,7 +351,12 @@ export const SlottedHeading = (args: StoryArgs): TemplateResult => {
quiet
></sp-textfield>
<div slot="subheading">Last modified on 6/17/2020, 3:37 PM</div>
<sp-action-menu slot="actions" placement="bottom-end" quiet>
<sp-action-menu
label="More Actions"
slot="actions"
placement="bottom-end"
quiet
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
Expand Down
25 changes: 20 additions & 5 deletions packages/coachmark/README.md
View file
Open in desktop
Expand Up @@ -56,7 +56,12 @@ Coach marks can include an `<sp-action-menu>`, which appears at the top right of
<div slot="content">
This is a Coachmark with nothing but text in it. Kind of lonely in here.
</div>
<sp-action-menu placement="bottom-end" quiet slot="actions">
<sp-action-menu
label="More Actions"
placement="bottom-end"
quiet
slot="actions"
>
<sp-menu-item>Skip tour</sp-menu-item>
<sp-menu-item>Restart tour</sp-menu-item>
</sp-action-menu>
Expand Down Expand Up @@ -86,7 +91,7 @@ The primary and secondary CTA buttons within the coachmark popover can be config
<div slot="content">
This is a Coachmark with nothing but text in it. Kind of lonely in here.
</div>
<sp-action-menu placement="bottom-end" quiet slot="actions">
<sp-action-menu label="More Actions" placement="bottom-end" quiet slot="actions">
<sp-menu-item>Skip tour</sp-menu-item>
<sp-menu-item>Restart tour</sp-menu-item>
</sp-action-menu>
Expand Down Expand Up @@ -137,7 +142,12 @@ Media Types allowed: `Images & Gifs`
>
<div slot="title">Coachmark with 16:9 image</div>
<div slot="content">This is a Coachmark with some description</div>
<sp-action-menu placement="bottom-end" quiet slot="actions">
<sp-action-menu
label="More Actions"
placement="bottom-end"
quiet
slot="actions"
>
<sp-menu-item>Skip tour</sp-menu-item>
<sp-menu-item>Restart tour</sp-menu-item>
</sp-action-menu>
Expand All @@ -159,7 +169,12 @@ A custom media can also be added via `<slot name="cover-photo"></slot>`
<div slot="title">Coachmark with 16:9 image</div>
<div slot="content">This is a Coachmark with some description</div>
<img slot="asset" src="https://picsum.photos/id/237/200/300" alt="" />
<sp-action-menu placement="bottom-end" quiet slot="actions">
<sp-action-menu
label="More Actions"
placement="bottom-end"
quiet
slot="actions"
>
<sp-menu-item>Skip tour</sp-menu-item>
<sp-menu-item>Restart tour</sp-menu-item>
</sp-action-menu>
Expand All @@ -182,7 +197,7 @@ The `shortcutKey` is the primary key used to trigger an interaction and are typi
secondary-cta="Previous"
id="coachmark-keys"
>
<sp-action-menu placement="bottom-end" quiet slot="actions">
<sp-action-menu label="More Actions" placement="bottom-end" quiet slot="actions">
<sp-menu-item>Skip tour</sp-menu-item>
<sp-menu-item>Restart tour</sp-menu-item>
</sp-action-menu>
Expand Down
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
6 changes: 5 additions & 1 deletion packages/menu/stories/submenu.stories.ts
View file
Open in desktop
Expand Up @@ -159,7 +159,11 @@ export const submenu = (): TemplateResult => {
valueEls.second.textContent = event.target.selected[0] || '';
};
return html`
<sp-action-menu @change=${handleRootChange} @sp-opened=${clearValues}>
<sp-action-menu
label="More Actions"
@change=${handleRootChange}
@sp-opened=${clearValues}
>
<sp-icon-show-menu slot="icon"></sp-icon-show-menu>
<sp-menu-group
@change=${() => console.log('group change')}
Expand Down
4 changes: 2 additions & 2 deletions packages/overlay/stories/overlay-element.stories.ts
View file
Open in desktop
Expand Up @@ -346,7 +346,7 @@ export const actionGroup = ({ delayed }: Properties): TemplateResult => {
<sp-action-button id="trigger-3" hold-affordance>
<sp-icon-rect-select slot="icon"></sp-icon-rect-select>
</sp-action-button>
<sp-action-menu placement="left">
<sp-action-menu label="More Actions" placement="left">
<sp-menu-group id="cms">
<span slot="header">cms</span>
<sp-menu-item value="updateAllSiteContent">
Expand Down Expand Up @@ -557,7 +557,7 @@ export const actionGroupWithFilters = ({
Hover
</sp-tooltip>
</sp-action-button>
<sp-action-menu>
<sp-action-menu label="More Actions">
<sp-menu-group id="cms">
<span slot="header">cms</span>
<sp-menu-item value="updateAllSiteContent">
Expand Down
26 changes: 18 additions & 8 deletions packages/toast/src/Toast.ts
View file
Open in desktop
Expand Up @@ -116,28 +116,38 @@ 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"
class="type"
></sp-icon-info>
<sp-icon-info label=${label} 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} class="type"></sp-icon-alert>
`;
case 'positive':
case 'success': // deprecated
return html`
<sp-icon-checkmark-circle
label="Success"
label=${label}
class="type"
></sp-icon-checkmark-circle>
`;
Expand Down Expand Up @@ -205,7 +215,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
5 changes: 2 additions & 3 deletions packages/toast/test/toast.test.ts
View file
Open in desktop
Expand Up @@ -56,14 +56,13 @@ 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();
});
});
Expand Down