fix(meter, progress-bar, progress-circle): use innerText when label is not provided #3483
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,17 +18,25 @@ import { | |
| SpectrumElement, | ||
| TemplateResult, | ||
| } from '@spectrum-web-components/base'; | ||
| import { | ||
| property, | ||
| query, | ||
| } from '@spectrum-web-components/base/src/decorators.js'; | ||
| import { ObserveSlotText } from '@spectrum-web-components/shared/src/observe-slot-text.js'; | ||
| import { getLabelFromSlot } from '@spectrum-web-components/shared/src/get-label-from-slot.js'; | ||
| import { ifDefined } from '@spectrum-web-components/base/src/directives.js'; | ||
|
|
||
| import progressCircleStyles from './progress-circle.css.js'; | ||
|
|
||
| /** | ||
| * @element sp-progress-circle | ||
| */ | ||
| export class ProgressCircle extends SizedMixin( | ||
Westbrook marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ObserveSlotText(SpectrumElement, ''), | ||
|
There was a problem hiding this comment. In that Progress Circle doesn't show the label visibly, it might not need this feature addition. It's not "free" from a performance standpoint, so we should be sure that it is 100% needed. Progress Bar may need it, as the label is visible there. There was a problem hiding this comment. what could be the other way of allowing the innerText to act as a label in case label is not provided without using a slot? There was a problem hiding this comment.
|
||
| { | ||
| validSizes: ['s', 'm', 'l'], | ||
TarunAdobe marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| ) { | ||
| public static override get styles(): CSSResultArray { | ||
| return [progressCircleStyles]; | ||
| } | ||
|
|
@@ -48,6 +56,9 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, { | |
| @property({ type: Number }) | ||
| public progress = 0; | ||
|
|
||
| @query('slot') | ||
| private slotEl!: HTMLSlotElement; | ||
|
|
||
| private makeRotation(rotation: number): string | undefined { | ||
| return this.indeterminate | ||
| ? undefined | ||
|
|
@@ -83,6 +94,7 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, { | |
| ]; | ||
| const masks = ['Mask1', 'Mask2']; | ||
| return html` | ||
| <slot @slotchange=${this.onSlotChange}></slot> | ||
| <div class="track"></div> | ||
| <div class="fills"> | ||
| ${masks.map( | ||
|
|
@@ -101,6 +113,17 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, { | |
| `; | ||
| } | ||
|
|
||
| protected onSlotChange(): void { | ||
| const labelFromSlot = getLabelFromSlot( | ||
| this.label, | ||
| this.slotHasContent, | ||
| this.slotEl | ||
| ); | ||
| if (labelFromSlot) { | ||
| this.label = labelFromSlot; | ||
| } | ||
| } | ||
|
|
||
Westbrook marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| protected override firstUpdated(changes: PropertyValues): void { | ||
| super.firstUpdated(changes); | ||
| if (!this.hasAttribute('role')) { | ||
|
|
@@ -115,24 +138,30 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, { | |
| } else if (this.hasAttribute('aria-valuenow')) { | ||
| this.removeAttribute('aria-valuenow'); | ||
| } | ||
| if (changes.has('label')) { | ||
| if (this.label.length) { | ||
| this.setAttribute('aria-label', this.label); | ||
| } else { | ||
| this.removeAttribute('aria-label'); | ||
| } | ||
| } | ||
|
|
||
| if (window.__swc.DEBUG) { | ||
| if ( | ||
| !this.label && | ||
| !this.getAttribute('aria-label') && | ||
| !this.getAttribute('aria-labelledby') && | ||
| !this.slotHasContent | ||
| ) { | ||
| window.__swc.warn( | ||
| this, | ||
| '<sp-progress-circle> elements need one of the following to be accessible:', | ||
| 'https://opensource.adobe.com/spectrum-web-components/components/progress-circle/#accessibility', | ||
| { | ||
| type: 'accessibility', | ||
| issues: [ | ||
| 'value supplied to the "label" attribute, which will be displayed visually as part of the element, or', | ||
| 'text content supplied directly to the <sp-progress-circle> element, or', | ||
| 'value supplied to the "aria-label" attribute, which will only be provided to screen readers, or', | ||
| 'an element ID reference supplied to the "aria-labelledby" attribute, which will be provided by screen readers and will need to be managed manually by the parent application.', | ||
| ], | ||
|
|
||
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Across the board here, 🙏🏼.
This method being used in each of three components points to this possibly benefiting a Reactive Controller or mixin for reusability. Let's not worry about that today, but keep an eye out for this pattern in case we end up using it again somewhere else.
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.
☝🏼
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.
We are using onSlotChange naming convention in multiple components already.... so this feels more consistent. for instance checkout -> ActiveOverlay.ts, Tabs.ts and TopNav.ts
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.
Thanks for digging in on normalization! That's a great trait, and something we should do more of as a project. However, we have not normalized here very well, see handleSlotchange usage... 😞
on*is a common practice in frameworks like React, and surfacing methods on our custom elements with a similar name can cause issues when working in that context without a wrapping React Component. We see this locally in our Storybook from time to time, and while we've recently started shipping React Wrappers to reduce the changes this happens for our consumers that leverage React, at some point in the future React will finally ship framework native support for custom events and setting properties on DOM elements and users will be freed to return to direct consumption of SWC in that context, so we want to be prepared for that.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.
alright got it. I'll change it then