fix(picker): add loading state to the picker #3110
Conversation
|
@Westbrook I opened a new PR, directly from the main repo (not fork). Checked your review from the previous PR, and from what I see, there are 2 things left:
|
Tachometer resultsChromeaction-bar permalinkbasic-test
action-menu permalinktest-basic
combobox permalinkbasic-test
light-dom-test permalink
menu permalinktest-basic
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
split-button permalinkbasic-test
tooltip permalinktest-basic
test-directive permalink
Firefoxaction-bar permalinkbasic-test
action-menu permalinktest-basic
combobox permalinkbasic-test
light-dom-test permalink
menu permalinktest-basic
overlay permalinkbasic-test
directive-test permalink
element-test permalink
lazy-test permalink
picker permalinkbasic-test
popover permalinktest-basic
split-button permalinkbasic-test
tooltip permalinktest-basic
test-directive permalink
|
Westbrook
left a comment
Westbrook
left a comment
There was a problem hiding this comment.
Great start. Mainly want to make sure we're shoring up the accessibility tree that this delivers and clarifying how we're preventing interaction. @jnurthen if you have any suggestions beyond what's here, would love your insight.
packages/picker/src/picker.css
Outdated
| --spectrum-picker-width: var(--spectrum-global-dimension-size-2400); | ||
| } | ||
|
|
||
| :host([loading]) sp-progress-circle { |
There was a problem hiding this comment.
@pfulton does this make sense? Is it something that we could maybe get added to the Spectrum CSS source for the future?
packages/picker/src/Picker.ts
Outdated
| ? html` | ||
| <sp-progress-circle | ||
| indeterminate | ||
| aria-labelledby="label" |
There was a problem hiding this comment.
aria-hidden below cancels this out, so we can exclude this line.
There was a problem hiding this comment.
sp-progress-circle throws a11y warnings if label is not provided, even when aria-hidden is present. Should we add a check for aria-hidden in this case too?
Westbrook
left a comment
Westbrook
left a comment
There was a problem hiding this comment.
@razvancir96 could you sync up with @TarunAdobe about normalizing this with the work in #3163 so we could maybe get this over the line?
|
After a discussion with @razvancir96 we agreed that our team will take over this PR from next week. |
|
@Rocss please sync up with @TarunAdobe about normalizing this PR and the API applied to the Picker herein with the work done in #3163. |
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
|
|
Given that the progress spinner is spinning inside the picker, VRT will fail because of different states of the spinner. Was there anything similar being done in the repo so far? |
|
For stability in the VRTs, checkout #4055. It should "just work ™️", 🤞🏼, after that PR lands. |
| type StoryArgs = { | ||
| onChange: (val: string) => void; | ||
| invalid: boolean; | ||
| pending: boolean; |
There was a problem hiding this comment.
@TarunAdobe we broken Button into a while list of sized stories, particularly due to the custom sizing of the Progress Circle. Have you found value in that? Should we do it here, too?
|
I have no more things to add to this PR. Happy to implement any other feedback if there is. |
|
Hey @Rocss I just have a final comment before we can land this... we'd want to split up the Picker into a list of sized stories so that we can track down how the pending progress-circle looks in different sizes just like we do in button. |
| type=${ifDefined(type)} | ||
| > | ||
| <sp-button variant="primary" slot="trigger">Show Popover</sp-button> | ||
| <sp-popover |
There was a problem hiding this comment.
The pre-commit hook automatically does this, it's not me!
There was a problem hiding this comment.
I wonder if there's a loose config somewhere. Not germain to this PR, indeed.
TarunAdobe
left a comment
TarunAdobe
left a comment
There was a problem hiding this comment.
LGTM! Tagging @Westbrook for one final pass before we land this.
Westbrook
changed the title
Westbrook
left a comment
Westbrook
left a comment
There was a problem hiding this comment.
Thanks for adopting this lost child of a PR and giving it a good home! 
| type=${ifDefined(type)} | ||
| > | ||
| <sp-button variant="primary" slot="trigger">Show Popover</sp-button> | ||
| <sp-popover |
There was a problem hiding this comment.
I wonder if there's a loose config somewhere. Not germain to this PR, indeed.
Description
We need to add a loading spinner inside the picker's button.
Spectrum CSS
Related issue(s)
Motivation and context
In one of Adobe's projects, we need to support the loading spinner inside the picker's button.
How has this been tested?
yarn storybookpendingproperty on true. Also, change the scale.Screenshots (if appropriate)
Size S:
Size M:
Size L:
Size XL:
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.