-
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?
Conversation
@@ -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 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
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.
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
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.
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.
Lighthouse scores
What is this?Lighthouse scores comparing the documentation site built from the PR ("Branch") to that of the production documentation site ("Latest") and the build currently on Transfer Size
Request Count
|
await elementUpdated(el); | ||
|
||
await expect(el.getAttribute('label')).to.equal('testLabel'); |
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.
I couldn't check if the sp-icon
's label in this test since sp-icon
isn't exposed in sp-toast
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.
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
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.
@sarahszhou Thank you, worked perfectly!
private _variant: ToastVariants = ''; | ||
|
||
private renderIcon(variant: string): TemplateResult { | ||
private _label: string = ''; |
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.
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?
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.
Agreed, if the consumer does not define a label, should we fall back to the existing strings?
private _label: string = ''; | |
<sp-icon-info label=${label === "" ? "Information" : label} class="type"></sp-icon-info> |
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.
Yeah falling back to the default makes sense to me!
Tachometer resultsChromeaccordion permalinkbasic-test
action-bar permalinkbasic-test
action-button permalinkbasic-test
action-group permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
alert-dialog permalinkbasic-test
button-group permalinkbasic-test
button permalinktest-basic
card permalinktest-basic
checkbox permalinktest-basic
coachmark permalinkbasic-test
color-field permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
dialog permalinkbasic-test
field-group permalinkbasic-test
field-label permalinkbasic-test
help-text permalinkbasic-test
icon permalinktest-basic
infield-button permalinkbasic-test
menu permalinktest-basic
meter permalinkbasic-test
number-field permalinkbasic-test
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker-button permalinkbasic-test
picker permalinkbasic-test
popover permalinktest-basic
progress-bar permalinkbasic-test
radio permalinktest-basic
search permalinktest-basic
slider permalinktest-basic
split-button permalinkbasic-test
swatch permalinkbasic-test
switch permalinktest-basic
table permalinkbasic-test
tabs permalinkbasic-test
tags permalinkbasic-test
textfield permalinktest-basic
toast permalinktest-basic
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
top-nav permalinkbasic-test
truncated permalinkbasic-test
Firefoxaccordion permalinkbasic-test
action-bar permalinkbasic-test
action-button permalinkbasic-test
action-group permalinkbasic-test
action-menu permalinktest-basic
test-directive permalink
test-lazy permalink
test-open-close-directive permalink
test-open-close permalink
alert-dialog permalinkbasic-test
button-group permalinkbasic-test
button permalinktest-basic
card permalinktest-basic
checkbox permalinktest-basic
coachmark permalinkbasic-test
color-field permalinkbasic-test
combobox permalinkbasic-test
light-dom-test permalink
dialog permalinkbasic-test
field-group permalinkbasic-test
field-label permalinkbasic-test
help-text permalinkbasic-test
icon permalinktest-basic
infield-button permalinkbasic-test
menu permalinktest-basic
meter permalinkbasic-test
number-field permalinkbasic-test
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker-button permalinkbasic-test
picker permalinkbasic-test
popover permalinktest-basic
progress-bar permalinkbasic-test
radio permalinktest-basic
search permalinktest-basic
slider permalinktest-basic
split-button permalinkbasic-test
swatch permalinkbasic-test
switch permalinktest-basic
table permalinkbasic-test
tabs permalinkbasic-test
tags permalinkbasic-test
textfield permalinktest-basic
toast permalinktest-basic
tooltip permalinktest-basic
test-directive permalink
test-element permalink
test-lazy permalink
top-nav permalinkbasic-test
truncated permalinkbasic-test
|
Hi @susmithayenugula This PR is pointing to |
@Rajdeepc I'll take care of this today, thank you! |
Hi @Rajdeepc , I've updated the PR as requested. Please take a look! |
@@ -26,7 +26,7 @@ export class IconBase extends SpectrumElement { | |||
return [iconStyles]; | |||
} | |||
|
|||
@property() | |||
@property({ type: String, reflect: true }) |
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?
@@ -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 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.
@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; | ||
} |
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.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
''
is falsy, so this can be simplified to label || 'Information'
.
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 ''; | ||
} | ||
}; |
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.
Can this be alleviated by the fact that the icon always has class="type"
?
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; | ||
} |
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.
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?
Added
label
prop to Toast that allows customization of thesp-icon
's label.Description
sp-icon
used in asp-toast
was hardcoded toSuccess
,Error
andInformation
forpositive
,negative
andinfo
toasts.Warning
for anegative
toast.label
prop and set value of this prop.Related issue(s)
fixes #3406
Motivation and context
The addition of the label prop allows one to customize the string. This is useful when we want to use the string
Warning
for anegative
toast.How has this been tested?
sp-icon-x
is insp-toast
's shadow-root.Controls
tab, locate the new attributelabel
.Remount component
in the top bar.label
prop ofsp-icon-x
.Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main
.