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(meter, progress-bar, progress-circle): use innerText when label is not provided #3483

Merged
merged 19 commits into from
Jul 27, 2023
Merged

fix(meter, progress-bar, progress-circle): use innerText when label is not provided #3483

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
5debd8e
fix(meter): use inner text content when explicit label is not there
Jul 21, 2023
62811f3
fix(progress-bar): fixed attribute update logic
Jul 21, 2023
6eb2d50
fix: updated slotChange logic
Jul 24, 2023
1ebe0c3
fix: created a helper function to get label from slot
Jul 24, 2023
6c93b20
removed unnecessary text
TarunAdobe Jul 24, 2023
d5dca86
fix: updated arguement types and warning condition logic for progress…
Jul 24, 2023
ac2c34b
Merge branch 'issue/3146-meter-progress-aria-label' of https://github…
Jul 24, 2023
c427759
fix: moved anonymous function to a class function
Jul 24, 2023
c016665
Merge branch 'main' into issue/3146-meter-progress-aria-label
TarunAdobe Jul 25, 2023
ad61446
fix(progress-bar): added a check for field-label component to pass th…
Jul 25, 2023
a321600
fix: updated swc warn message
Jul 25, 2023
1c05f55
Update packages/progress-bar/src/ProgressBar.ts
TarunAdobe Jul 25, 2023
16337a3
fix: added a few tests to improve the code coverage
Jul 27, 2023
d3ddb18
fix: removed extraspaces from slot content
Jul 27, 2023
08af668
fix: removed unnecessary branching conditions
Jul 27, 2023
2e17b8e
Merge branch 'main' into issue/3146-meter-progress-aria-label
TarunAdobe Jul 27, 2023
7932e0c
fix: removed ObserveSlotText from progress circle and updated functio…
Jul 27, 2023
3bc3a04
Update ProgressCircle.ts
TarunAdobe Jul 27, 2023
01201f3
fix(progress-circle): improved performance by removing slot has conte…
TarunAdobe Jul 27, 2023
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
30 changes: 26 additions & 4 deletions packages/meter/src/Meter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,12 @@ import {
SpectrumElement,
TemplateResult,
} from '@spectrum-web-components/base';
import { property } from '@spectrum-web-components/base/src/decorators.js';
import {
property,
query,
} from '@spectrum-web-components/base/src/decorators.js';

import { getLabelFromSlot } from '@spectrum-web-components/shared/src/get-label-from-slot.js';
import { ObserveSlotText } from '@spectrum-web-components/shared/src/observe-slot-text.js';
import { LanguageResolutionController } from '@spectrum-web-components/reactive-controllers/src/LanguageResolution.js';
import '@spectrum-web-components/field-label/sp-field-label.js';
Expand Down Expand Up @@ -53,6 +57,9 @@ export class Meter extends SizedMixin(ObserveSlotText(SpectrumElement, '')) {
@property({ type: String, reflect: true })
public label = '';

@query('slot')
private slotEl!: HTMLSlotElement;

private languageResolver = new LanguageResolutionController(this);

@property({ type: Boolean, reflect: true, attribute: 'side-label' })
Expand All @@ -66,7 +73,7 @@ export class Meter extends SizedMixin(ObserveSlotText(SpectrumElement, '')) {
return html`
<sp-field-label size=${this.size} class="label">
${this.slotHasContent ? html`` : this.label}
<slot>${this.label}</slot>
<slot @slotchange=${this.handleSlotchange}>${this.label}</slot>
</sp-field-label>
<sp-field-label size=${this.size} class="percentage">
${new Intl.NumberFormat(this.languageResolver.language, {
Expand All @@ -83,6 +90,17 @@ export class Meter extends SizedMixin(ObserveSlotText(SpectrumElement, '')) {
`;
}

protected handleSlotchange(): void {
const labelFromSlot = getLabelFromSlot(
this.label,
this.slotHasContent,
this.slotEl
);
if (labelFromSlot) {
this.label = labelFromSlot;
}
}

protected override firstUpdated(changes: PropertyValues): void {
super.firstUpdated(changes);
this.setAttribute('role', 'meter progressbar');
Expand All @@ -93,8 +111,12 @@ export class Meter extends SizedMixin(ObserveSlotText(SpectrumElement, '')) {
if (changes.has('progress')) {
this.setAttribute('aria-valuenow', '' + this.progress);
}
if (this.label && changes.has('label')) {
this.setAttribute('aria-label', this.label);
if (changes.has('label')) {
if (this.label.length) {
this.setAttribute('aria-label', this.label);
} else {
this.removeAttribute('aria-label');
}
}
}
}
9 changes: 9 additions & 0 deletions packages/meter/test/meter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,15 @@ describe('Meter', () => {
expect(el.getAttribute('aria-valuenow')).to.equal('100');
});

it('accepts label from `slot`', async () => {
const el = await fixture<Meter>(html`
<sp-meter>Label From Slot</sp-meter>
`);

await elementUpdated(el);

expect(el.getAttribute('label')).to.equal('Label From Slot');
});
it('accepts a changing process', async () => {
const el = await fixture<Meter>(html`
<sp-meter>Changing Value</sp-meter>
Expand Down
55 changes: 44 additions & 11 deletions packages/progress-bar/src/ProgressBar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,31 @@ import {
SpectrumElement,
TemplateResult,
} from '@spectrum-web-components/base';
import { property } from '@spectrum-web-components/base/src/decorators.js';
import {
property,
query,
} from '@spectrum-web-components/base/src/decorators.js';

import { getLabelFromSlot } from '@spectrum-web-components/shared/src/get-label-from-slot.js';
import { ObserveSlotText } from '@spectrum-web-components/shared/src/observe-slot-text.js';
import { LanguageResolutionController } from '@spectrum-web-components/reactive-controllers/src/LanguageResolution.js';
import '@spectrum-web-components/field-label/sp-field-label.js';
import styles from './progress-bar.css.js';

/**
* @element sp-progress-bar
*/
export class ProgressBar extends SizedMixin(SpectrumElement) {
export class ProgressBar extends SizedMixin(
ObserveSlotText(SpectrumElement, '')
) {
public static override get styles(): CSSResultArray {
return [styles];
}

@property({ type: Boolean, reflect: true })
public indeterminate = false;

@property({ type: String })
@property({ type: String, reflect: true })
public label = '';

private languageResolver = new LanguageResolutionController(this);
Expand All @@ -52,13 +59,23 @@ export class ProgressBar extends SizedMixin(SpectrumElement) {
@property({ type: String, reflect: true })
public static: 'white' | undefined;

@query('slot')
private slotEl!: HTMLSlotElement;

protected override render(): TemplateResult {
return html`
${this.label
${this.slotHasContent || this.label
? html`
<sp-field-label size=${this.size} class="label">
${this.label}
${this.slotHasContent ? html`` : this.label}
<slot @slotchange=${this.handleSlotchange}>
${this.label}
</slot>
</sp-field-label>
`
: html``}
${this.label
? html`
${this.indeterminate
? html``
: html`
Expand Down Expand Up @@ -86,6 +103,17 @@ export class ProgressBar extends SizedMixin(SpectrumElement) {
`;
}

protected handleSlotchange(): void {
const labelFromSlot = getLabelFromSlot(
this.label,
this.slotHasContent,
this.slotEl
);
if (labelFromSlot) {
this.label = labelFromSlot;
}
}

protected override firstUpdated(changes: PropertyValues): void {
super.firstUpdated(changes);
if (!this.hasAttribute('role')) {
Expand All @@ -99,34 +127,39 @@ export class ProgressBar extends SizedMixin(SpectrumElement) {
if (this.indeterminate) {
this.removeAttribute('aria-valuemin');
this.removeAttribute('aria-valuemax');
this.removeAttribute('aria-valuenow');
Rajdeepc marked this conversation as resolved.
Show resolved Hide resolved
} else {
this.setAttribute('aria-valuemin', '0');
this.setAttribute('aria-valuemax', '100');
}
}
if (!this.indeterminate && changes.has('progress')) {
this.setAttribute('aria-valuenow', '' + this.progress);
} else if (this.hasAttribute('aria-valuenow')) {
this.removeAttribute('aria-valuenow');
}
if (this.label && changes.has('label')) {
this.setAttribute('aria-label', this.label);
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.getAttribute('aria-labelledby') &&
!this.slotHasContent
) {
Westbrook marked this conversation as resolved.
Show resolved Hide resolved
window.__swc.warn(
this,
'<sp-progress-bar> elements will not be accessible to screen readers without one of the following:',
'<sp-progress-bar> elements need one of the following to be accessible:',
'https://opensource.adobe.com/spectrum-web-components/components/progress-bar/#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.',
],
Expand Down
11 changes: 11 additions & 0 deletions packages/progress-bar/test/progress-bar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ describe('ProgressBar', () => {
await expect(el).to.be.accessible();
});

it('accepts label from `slot`', async () => {
const el = await fixture<ProgressBar>(html`
<sp-progress-bar role="progressbar">
Label From Slot
</sp-progress-bar>
`);

await elementUpdated(el);

expect(el.getAttribute('label')).to.equal('Label From Slot');
});
it('accepts a changing progress', async () => {
const el = await fixture<ProgressBar>(html`
<sp-progress-bar label="Changing value"></sp-progress-bar>
Expand Down
39 changes: 34 additions & 5 deletions packages/progress-circle/src/ProgressCircle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ import {
SpectrumElement,
TemplateResult,
} from '@spectrum-web-components/base';
import { property } from '@spectrum-web-components/base/src/decorators.js';
import {
property,
query,
} from '@spectrum-web-components/base/src/decorators.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';
Expand Down Expand Up @@ -48,6 +52,12 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, {
@property({ type: Number })
public progress = 0;

@query('slot')
private slotEl!: HTMLSlotElement;

@property({ type: Boolean })
private slotHasContent = false;

private makeRotation(rotation: number): string | undefined {
return this.indeterminate
? undefined
Expand Down Expand Up @@ -83,6 +93,7 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, {
];
const masks = ['Mask1', 'Mask2'];
return html`
<slot @slotchange=${this.handleSlotchange}></slot>
<div class="track"></div>
<div class="fills">
${masks.map(
Expand All @@ -101,11 +112,23 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, {
`;
}

protected handleSlotchange(): 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')) {
this.setAttribute('role', 'progressbar');
}
this.slotHasContent = this.slotEl.assignedNodes().length > 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the work that I was attempting to do in a lazy manner by removing the ObserveSlotText mixin from this class. Below in getLabelFromSlot you already do a check for this, so if you were to remove slotHasContent from the method signature there you would be able to do less work in this element unless it was needed, as signified by the lack of a 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.

so should I remove the slotHasContent all together from the method itself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the functionality continues as expected without it, yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Im running into a small issue here... If I remove the slotHasContent Attribute then in the dev warnings the condition to give warning would be true if the label is not there. So let's say someone has provided some content in the slot but still that condition would be true for the first render call and then it would be false as the label would get updated from the slot content. But this also means the warning would still be there in console from the first call. That is why I added this variable to check if the slot has content then we don't show the warning even if the label is not there bcz we know eventually label would be updated with the slot content once the onSlotChange method is completed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super helpful delineation!

ObserveSlotText will always do some work, whether you have content or not, have a label or not, have Dev Mode or not...

Any work within if (window.__swc.DEBUG) { ... } will only happen when Dev Mode is on. So if we replace !this.slotHasContent in line 154 with !this.slotEl.assignedNodes().length we'd only require using in that context to do that work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it... I'll fix it and make a commit now... Thank you so much for clarifying

}

protected override updated(changes: PropertyValues): void {
Expand All @@ -115,24 +138,30 @@ export class ProgressCircle extends SizedMixin(SpectrumElement, {
} else if (this.hasAttribute('aria-valuenow')) {
this.removeAttribute('aria-valuenow');
}
if (this.label && changes.has('label')) {
this.setAttribute('aria-label', this.label);
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.getAttribute('aria-labelledby') &&
!this.slotHasContent
) {
window.__swc.warn(
this,
'<sp-progress-circle> elements will not be accessible to screen readers without one of the following:',
'<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.',
],
Expand Down
4 changes: 4 additions & 0 deletions packages/progress-circle/src/progress-circle.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,7 @@ governing permissions and limitations under the License.
--spectrum-alias-track-fill-color-overbackground
);
}

slot {
display: none;
}
12 changes: 6 additions & 6 deletions packages/progress-circle/stories/progress-circle.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,18 +47,18 @@ export const Default = ({ indeterminate }: StoryArgs = {}): TemplateResult => {
progress="27"
size="s"
?indeterminate=${indeterminate}
label="Loading progess demo"
label="Loading progress demo"
></sp-progress-circle>
<sp-progress-circle
progress="27"
?indeterminate=${indeterminate}
label="Loading progess demo"
label="Loading progress demo"
></sp-progress-circle>
<sp-progress-circle
progress="27"
size="l"
?indeterminate=${indeterminate}
label="Loading progess demo"
label="Loading progress demo"
></sp-progress-circle>
</div>
`;
Expand All @@ -79,20 +79,20 @@ export const staticWhite = ({
static="white"
size="s"
?indeterminate=${indeterminate}
label="Loading progess demo"
label="Loading progress demo"
></sp-progress-circle>
<sp-progress-circle
progress="53"
static="white"
?indeterminate=${indeterminate}
label="Loading progess demo"
label="Loading progress demo"
></sp-progress-circle>
<sp-progress-circle
progress="53"
static="white"
size="l"
?indeterminate=${indeterminate}
label="Loading progess demo"
label="Loading progress demo"
Westbrook marked this conversation as resolved.
Show resolved Hide resolved
></sp-progress-circle>
</div>
`;
Expand Down
11 changes: 11 additions & 0 deletions packages/progress-circle/test/progress-circle.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,17 @@ describe('ProgressCircle', () => {

await expect(el).to.be.accessible();
});
it('accepts label from `slot`', async () => {
const el = await fixture<ProgressCircle>(html`
<sp-progress-circle role="progressbar">
Label From Slot
</sp-progress-circle>
`);

await elementUpdated(el);

expect(el.getAttribute('aria-label')).to.equal('Label From Slot');
});
it('accepts user `role`', async () => {
const el = await fixture<ProgressCircle>(html`
<sp-progress-circle
Expand Down