-
Notifications
You must be signed in to change notification settings - Fork 191
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.
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
fix(toast): Toast should have API for overriding the icon's alt text #4159
base: main
Are you sure you want to change the base?
Changes from all commits
fd0b6a4
5fe626c
c4cdb8c
7f4b08e
3051037
aef34dc
913d15e
99bf2ff
1ddb7b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ export class IconBase extends SpectrumElement { | |
return [iconStyles]; | ||
} | ||
|
||
@property() | ||
@property({ type: String, reflect: true }) | ||
public label = ''; | ||
|
||
@property({ reflect: true }) | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder why we don't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the API level we generally use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great add. However, this whole |
||
} else { | ||
this.setAttribute('aria-hidden', 'true'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we not use a simple decorator here:
And then avoid all the rest of the manual administration? As far as naming, this isn't so much the |
||||||
|
||||||
private _variant: ToastVariants = ''; | ||||||
|
||||||
private renderIcon(variant: string): TemplateResult { | ||||||
private _label: string = ''; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
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> | ||||||
`; | ||||||
|
@@ -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> | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't check if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you try getting it through the
I see that used in https://github.com/adobe/spectrum-web-components/blob/main/packages/tray/test/tray.test.ts#L105 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be alleviated by the fact that the icon always has |
||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some of this can be simplified via |
||
|
||
await elementUpdated(el); | ||
const icon = el.shadowRoot.querySelector(iconType) as HTMLElement; | ||
await expect(icon.getAttribute('label')).to.equal(defaultLabel); | ||
}); | ||
}); | ||
it('loads - timeout', async () => { | ||
|
There was a problem hiding this comment.
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?