fix(toast): Toast should have API for overriding the icon's alt text #4159
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
susmithayenugula
commented
Mar 8, 2024
| @@ -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.
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.
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.
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
|
susmithayenugula
commented
Mar 8, 2024
| await elementUpdated(el); | ||
|
|
||
| await expect(el.getAttribute('label')).to.equal('testLabel'); |
There was a problem hiding this comment.
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.
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.
@sarahszhou Thank you, worked perfectly!
susmithayenugula
commented
Mar 8, 2024
| private _variant: ToastVariants = ''; | ||
|
|
||
| private renderIcon(variant: string): TemplateResult { | ||
| private _label: string = ''; |
There was a problem hiding this comment.
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.
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> |
There was a problem hiding this comment.
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
|
added 2 commits
March 15, 2024 13:43
|
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! |
Westbrook
reviewed
Apr 2, 2024
| @@ -26,7 +26,7 @@ export class IconBase extends SpectrumElement { | |||
| return [iconStyles]; | |||
| } | |||
|
|
|||
| @property() | |||
| @property({ type: String, reflect: true }) | |||
There was a problem hiding this comment.
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.
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.
Comment on lines
+119
to
+128
| @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.
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.
'' is falsy, so this can be simplified to label || 'Information'.
Comment on lines
+78
to
+91
| 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.
Can this be alleviated by the fact that the icon always has class="type"?
Comment on lines
+109
to
+130
| 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.
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?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added
labelprop to Toast that allows customization of thesp-icon's label.Description
sp-iconused in asp-toastwas hardcoded toSuccess,ErrorandInformationforpositive,negativeandinfotoasts.Warningfor anegativetoast.labelprop 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
Warningfor anegativetoast.How has this been tested?
sp-icon-xis insp-toast's shadow-root.Controlstab, locate the new attributelabel.Remount componentin the top bar.labelprop 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.