Skip to content

feat(picker): add loading state to the picker #3093

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

Closed
wants to merge 1 commit into from
Closed

feat(picker): add loading state to the picker #3093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 39 additions & 1 deletion packages/picker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ When the `value` of an `<sp-picker>` matches the `value` attribute or the trimme
</sp-picker>
<br />
<br />
<sp-field-label for="picker-disabled">Quiet:</sp-field-label>
<sp-field-label for="picker-disabled-quiet">Quiet:</sp-field-label>
<sp-picker
label="Select a Country with a very long label, too long in fact"
disabled
Expand All @@ -393,6 +393,44 @@ When the `value` of an `<sp-picker>` matches the `value` attribute or the trimme
</sp-picker>
```

### Loading

When given a `loading` attribute, an `<sp-picker>` will be delivered with an `<sp-progress-circle>` to visually outline that it is `loading` and it will not toggle open or display its `<sp-menu-item>` descendants until the attribute is removed.

```html
<sp-field-label for="picker-loading">Standard:</sp-field-label>
<sp-picker
label="Select a Country with a very long label, too long in fact"
loading
id="picker-loading"
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
<sp-menu-item>Select and mask...</sp-menu-item>
<sp-menu-divider></sp-menu-divider>
<sp-menu-item>Save selection</sp-menu-item>
<sp-menu-item disabled>Make work path</sp-menu-item>
</sp-picker>
<br />
<br />
<sp-field-label for="picker-loading-quiet">Quiet:</sp-field-label>
<sp-picker
label="Select a Country with a very long label, too long in fact"
loading
quiet
id="picker-loading-quiet"
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
<sp-menu-item>Select and mask...</sp-menu-item>
<sp-menu-divider></sp-menu-divider>
<sp-menu-item>Save selection</sp-menu-item>
<sp-menu-item disabled>Make work path</sp-menu-item>
</sp-picker>
```

## Accessibility

To render accessibly, an `<sp-picker>` element should be paired with an `<sp-field-label>` element that has a `for` attribute referencing the `id` of the `<sp-picker>` element.
1 change: 1 addition & 0 deletions packages/picker/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@
"@spectrum-web-components/menu": "^0.16.15",
"@spectrum-web-components/overlay": "^0.19.3",
"@spectrum-web-components/popover": "^0.12.15",
"@spectrum-web-components/progress-circle": "^0.7.7",
"@spectrum-web-components/reactive-controllers": "^0.3.5",
"@spectrum-web-components/shared": "^0.15.5",
"@spectrum-web-components/tray": "^0.5.1"
Expand Down
17 changes: 16 additions & 1 deletion packages/picker/src/Picker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ import type {
} from '@spectrum-web-components/menu';
import '@spectrum-web-components/tray/sp-tray.js';
import '@spectrum-web-components/popover/sp-popover.js';
import '@spectrum-web-components/progress-circle/sp-progress-circle.js';

import type { Popover } from '@spectrum-web-components/popover';
import {
openOverlay,
Expand Down Expand Up @@ -105,6 +107,9 @@ export class PickerBase extends SizedMixin(Focusable) {
@property({ type: Boolean, reflect: true })
public invalid = false;

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

@property()
public label?: string;

Expand Down Expand Up @@ -391,6 +396,7 @@ export class PickerBase extends SizedMixin(Focusable) {
'visually-hidden': this.icons === 'only' && !!this.value,
placeholder: !this.value,
};

return [
html`
<span id="icon" ?hidden=${this.icons === 'none'}>
Expand All @@ -406,6 +412,15 @@ export class PickerBase extends SizedMixin(Focusable) {
></sp-icon-alert>
`
: nothing}
${this.loading
? html`
<sp-progress-circle
indeterminate
aria-labelledby="label"
aria-hidden="true"
></sp-progress-circle>
`
: nothing}
<sp-icon-chevron100
class="picker ${chevronClass[
this.size as DefaultElementSize
Expand Down Expand Up @@ -433,7 +448,7 @@ export class PickerBase extends SizedMixin(Focusable) {
@blur=${this.onButtonBlur}
@click=${this.onButtonClick}
@focus=${this.onButtonFocus}
?disabled=${this.disabled}
?disabled=${this.disabled || this.loading}
tabindex="-1"
>
${this.buttonContent}
Expand Down
35 changes: 35 additions & 0 deletions packages/picker/src/picker.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,41 @@ governing permissions and limitations under the License.
--spectrum-picker-width: var(--spectrum-global-dimension-size-2400);
}

:host([loading]) sp-progress-circle {
margin-inline-start: var(
--mod-picker-spacing-text-to-alert-icon-inline-start,
var(--spectrum-picker-spacing-text-to-alert-icon-inline-start)
);
}

:host([size='s']) sp-progress-circle {
--spectrum-progress-circle-size: var(--spectrum-workflow-icon-size-75);
--spectrum-progress-circle-thickness: var(
--spectrum-progress-circle-thickness-small
);
}
Copy link
Contributor Author

@razvancir96 razvancir96 Apr 6, 2023

Choose a reason for hiding this comment

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

Should I pick some other variables? Or make my own ones? More context: the picker has 4 dimensions: s/m/l/xl and the loading spinner has 3 dimensions: s/m/l, and do not match together. So I had to adapt its dimensions. Also, should we validate with design those dimensions?

Copy link
Contributor

@Westbrook Westbrook Apr 10, 2023

Choose a reason for hiding this comment

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

@pfulton is there any token support for these variation?

Copy link
Contributor

@Westbrook Westbrook Apr 10, 2023

Choose a reason for hiding this comment

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

From a selector standpoint, we can use :host([size="s"]) sp-progress-circle instead of needing to add a dynamic class to this element.


:host([size='m']) sp-progress-circle {
--spectrum-progress-circle-size: var(--spectrum-workflow-icon-size-100);
--spectrum-progress-circle-thickness: var(
--spectrum-progress-circle-thickness-small
);
}

:host([size='l']) sp-progress-circle {
--spectrum-progress-circle-size: var(--spectrum-workflow-icon-size-200);
--spectrum-progress-circle-thickness: var(
--spectrum-progress-circle-thickness-medium
);
}

:host([size='xl']) sp-progress-circle {
--spectrum-progress-circle-size: var(--spectrum-workflow-icon-size-300);
--spectrum-progress-circle-thickness: var(
--spectrum-progress-circle-thickness-large
);
}

#button {
width: 100%;
min-width: 100%;
Expand Down
30 changes: 30 additions & 0 deletions packages/picker/stories/picker-sizes.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,19 +22,47 @@ export default {
component: 'sp-picker',
argTypes: {
onChange: { action: 'change' },
invalid: {
name: 'invalid',
type: { name: 'boolean', required: false },
table: {
type: { summary: 'boolean' },
defaultValue: { summary: false },
},
control: {
type: 'boolean',
},
},
loading: {
name: 'loading',
type: { name: 'boolean', required: false },
table: {
type: { summary: 'boolean' },
defaultValue: { summary: false },
},
control: {
type: 'boolean',
},
},
},
};

type StoryArgs = {
onChange: (val: string) => void;
invalid: boolean;
loading: boolean;
};

const picker = ({
onChange,
size,
loading,
invalid,
}: {
onChange: (val: string) => void;
size: 's' | 'm' | 'l' | 'xl';
loading: boolean;
invalid: boolean;
}): TemplateResult => {
return html`
<sp-field-label for="picker-${size}" size=${size}>
Expand All @@ -48,6 +76,8 @@ const picker = ({
onChange(picker.value);
}}"
label="Select a Country with a very long label, too long, in fact"
?loading="${loading}"
?invalid="${invalid}"
>
<sp-menu-item>Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
Expand Down
34 changes: 34 additions & 0 deletions packages/picker/stories/picker.stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,17 @@ export default {
type: 'boolean',
},
},
loading: {
name: 'loading',
type: { name: 'boolean', required: false },
table: {
type: { summary: 'boolean' },
defaultValue: { summary: false },
},
control: {
type: 'boolean',
},
},
open: {
name: 'open',
type: { name: 'boolean', required: false },
Expand Down Expand Up @@ -405,6 +416,29 @@ export const custom = (args: StoryArgs): TemplateResult => {
`;
};

export const Loading = (args: StoryArgs): TemplateResult => {
return html`
<sp-field-label for="picker-loading">
Picker in loading state
</sp-field-label>
<sp-picker
id="picker-loading"
@change=${handleChange(args)}
${spreadProps(args)}
loading
>
<span slot="label">Loading...</span>
<sp-menu-item value="item-1">Deselect</sp-menu-item>
<sp-menu-item>Select Inverse</sp-menu-item>
<sp-menu-item>Feather...</sp-menu-item>
<sp-menu-item>Select and Mask...</sp-menu-item>
<sp-menu-divider></sp-menu-divider>
<sp-menu-item>Save Selection</sp-menu-item>
<sp-menu-item disabled>Make Work Path</sp-menu-item>
</sp-picker>
`;
};

custom.args = {
open: true,
};
7 changes: 7 additions & 0 deletions packages/picker/test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,13 @@ export function runPickerTests(): void {
expect(el.invalid).to.be.true;
await expect(el).to.be.accessible();
});
it('renders loading accessibly', async () => {
el.loading = true;
await elementUpdated(el);

expect(el.loading).to.be.true;
await expect(el).to.be.accessible();
});
Copy link
Contributor Author

@razvancir96 razvancir96 Apr 6, 2023

Choose a reason for hiding this comment

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

I copied the pattern from the invalid property, but I feel like some more tests should be added. Are there any guidelines about writing tests?

Copy link
Contributor

@Westbrook Westbrook Apr 10, 2023

Choose a reason for hiding this comment

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

Two things we'll want to test for:

  • is the listbox disabled when loading: load, apply loading, try to click, see that open === false and the Menu Items cannot be clicked with sendMouse
  • is the final "loading" label applied to the accessibility tree: load, apply loading, get an accessibility snapshot, ensure there is a node with the expected label

Copy link
Contributor Author

@razvancir96 razvancir96 Apr 10, 2023

Choose a reason for hiding this comment

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

Thanks! One additional question about testing: is there any command to run only the tests from one file? I tried some combinations, but failed 😅

Copy link
Contributor

@Westbrook Westbrook Apr 10, 2023

Choose a reason for hiding this comment

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

yarn test:watch:focus picker

it('renders selection accessibly', async () => {
el.value = 'option-2';
await elementUpdated(el);
Expand Down