Move Picker's state icon in a method, so that it could be overridden from outside

[razvancir96]

Code of conduct

• I agree to follow this project's code of conduct.

Description of issue

We need to add a loading spinner inside the picker's button, which is not supported right now. There is an invalid icon supported and we can extract that logic in a separate method, which can be overridden from outside. This way, the component's API and DOM structure isn't changed, but the consumers gain more flexibility.

What we have now (in render):

${this.invalid
        ? html`
              <sp-icon-alert
                  class="validation-icon"
              ></sp-icon-alert>
          `
        : nothing}

The more flexible version:

protected renderStateIcon(): TemplateResult | typeof nothing {
    if (!this.invalid) {
        return nothing;
    }
    return html`
        <sp-icon-alert class="validation-icon"></sp-icon-alert>
    `;
}

In render:
${this.renderStateIcon()}

[Westbrook]

Loading UI has recently been added to the CSS implementation. It would be preferrable that we add support for this directly to the element. Thoughts on how you would approach that here?

[razvancir96]

Loading UI has recently been added to the CSS implementation. It would be preferrable that we add support for this directly to the element. Thoughts on how you would approach that here?

Thanks for pointing it out! Sounds good to implement it directly in this repo. There are 2 things that I want to point out:

• Spectrum's Picker page has no specifications about the loading state (also checked the XD file)
• The CSS implementation, for the is-loading class, it only applies a disabled style to the arrow down icon. But with the CSS approach, the components can be easily composed.

Those being said, we can definitely add a new property to the picker, called loading, which decides to show/hide the loading spinner. But I'm not sure if we should do some other things when setting the property, or let the user decide if the picker is disabled or other things. Thoughts on this?

[Westbrook]

That's a great question!

loading feels like it would be sometimes disabled, but not always, so I think leaving that to the consuming developer makes sense for right now. I'll also bring this up with the Spectrum team when we sync with them this week and see if there isn't any further advice they can share. There is a slight but more info in the internal page, and I can get them to cross reference each those as part of that discussion.

For now starting with loading to support the CSS delivery of .is-loading should be an acceptable unit of work. In conjunction with that, it'll be important to get a story added to Storybook so that this will be tracked via our VRTs going forward. Be sure to checkout our docs on that process.

[Westbrook]

For this initial implementation, a loading Picker should not make its Menu available to a user, so it is effectively "disabled".

[najikahalsema]

wontfix