Add loading content parameter

[owenniblock]

Authors: Please fill out this form carefully and completely.

Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.

What are you trying to accomplish?

Adds a loading_content slot to SelectPanel to allow the loading Spinner to be overridden with custom content.

Integration

No

List the issues that this change affects.

Closes https://github.com/github/primer/issues/3926

Risk Assessment

• Low risk the change is small, highly observable, and easily rolled back.
• Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
• High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

Until the feature is used, it shouldn't affect other functionality so marking this as low risk.

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

I added the information to the .json files that reference the previews, I don't know if that's how you do it or if there's some automated process for this, let me know if there's some other way to do this.

Also I didn't add a test for this, I figure that because the loading content goes away eventually, it's likely to cause the test to be flaky. Maybe there's a way to short-circuit the loading process so that the loading spinner remains visible forever, but I felt it was better to just get this out there as it's a pretty simple change.

Accessibility

• Fixes axe scan violation - This change fixes an existing axe scan violation.
• No new axe scan violation - This change does not introduce any new axe scan violations.
• New axe violation - This change introduces a new axe scan violation. Please describe why the violation cannot be resolved below.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Lookbook)
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7e444f1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/view-components	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

[camertron]

Awesome, thanks @owenniblock 😄

Can we change this a bit to always render a spinner? The design feedback mentioned the ability to customize the loading text only.

[owenniblock]

@camertron if it's just the text we want to update here, would it make more sense to have this as a parameter on the SelectPanel class? Something like:

      # @param src [String] The URL to fetch search results from.
      # @param title [String] The title that appears at the top of the panel.
      # @param id [String] The unique ID of the panel.
      # @param size [Symbol] The size of the panel. <%= one_of(Primer::Alpha::Overlay::SIZE_OPTIONS) %>
      # @param select_variant [Symbol] <%= one_of(Primer::Alpha::SelectPanel::SELECT_VARIANT_OPTIONS) %>
      # @param fetch_strategy [Symbol] <%= one_of(Primer::Alpha::SelectPanel::FETCH_STRATEGIES) %>
      # @param no_results_label [String] The label to display when no results are found.
      # @param preload [Boolean] Whether to preload search results when the page loads. If this option is false, results are loaded when the panel is opened.
      # @param dynamic_label [Boolean] Whether or not to display the text of the currently selected item in the show button.
      # @param dynamic_label_prefix [String] If provided, the prefix is prepended to the dynamic label and displayed in the show button.
      # @param dynamic_aria_label_prefix [String] If provided, the prefix is prepended to the dynamic label and set as the value of the `aria-label` attribute on the show button.
      # @param body_id [String] The unique ID of the panel body. If not provided, the body ID will be set to the panel ID with a "-body" suffix.
      # @param list_arguments [Hash] Arguments to pass to the underlying <%= link_to_component(Primer::Alpha::ActionList) %> component. Only has an effect for the local fetch strategy.
      # @param form_arguments [Hash] Form arguments to pass to the underlying <%= link_to_component(Primer::Alpha::ActionList) %> component. Only has an effect for the local fetch strategy.
      # @param show_filter [Boolean] Whether or not to show the filter input.
      # @param open_on_load [Boolean] Open the panel when the page loads.
      # @param anchor_align [Symbol] The anchor alignment of the Overlay. <%= one_of(Primer::Alpha::Overlay::ANCHOR_ALIGN_OPTIONS) %>
      # @param anchor_side [Symbol] The side to anchor the Overlay to. <%= one_of(Primer::Alpha::Overlay::ANCHOR_SIDE_OPTIONS) %>
      # @param loading_message [String] Loading text which is added to the `aria-label` of the Spinner element
      # @param system_arguments [Hash] <%= link_to_system_arguments_docs %>
      def initialize(
        src: nil,
        title: "Menu",
        id: self.class.generate_id,
        size: :small,
        select_variant: DEFAULT_SELECT_VARIANT,
        fetch_strategy: DEFAULT_FETCH_STRATEGY,
        no_results_label: "No results found",
        preload: DEFAULT_PRELOAD,
        dynamic_label: false,
        dynamic_label_prefix: nil,
        dynamic_aria_label_prefix: nil,
        body_id: nil,
        list_arguments: {},
        form_arguments: {},
        show_filter: true,
        open_on_load: false,
        anchor_align: Primer::Alpha::Overlay::DEFAULT_ANCHOR_ALIGN,
        anchor_side: Primer::Alpha::Overlay::DEFAULT_ANCHOR_SIDE,
        loading_message: "Loading content..."
        **system_arguments
      )

If the idea is that we also might want to display the text visually, then we might want to change the Spinner markup so that the Spinner element itself is hidden from the screen reader and the text is available. But if that's optional, then we're going to want to also add a Boolean parameter to switch this on and off.

[camertron]

Hmm yeah we discussed that a bit yesterday... I was leaning toward a free-form slot because that's what we do for error messaging, but your concern about hiding the spinner from screen readers coupled with labeling concerns (eg. the spinner should probably have an aria-describedby that points to the error text) is making me reconsider. Error messages don't really have the same needs since it's not necessary to link two UI elements together there.

Ok, let's make the loading message a parameter, I think you're right.

Re: aria-* attributes: we could add these ourselves, but I'm not 100% confident I know the right course of action here, so I'm inclined to wait for an a11y review.

[owenniblock]

Cool, thanks @camertron - I can work out what's best for the aria-* stuff. I just re-read the original comment from @maximedegreve and I wonder if my thoughts on what he's after here are not quite right. I was thinking we wanted to be able to customise the loading text itself (which is currently added as an aria-label to the Spinner element itself.

However rereading the original ask:

Could we also provide a slot to add a description for the error and loading state?

Maybe we want to add additional information here?

My original change (this PR) will allow all of these scenarios but at the cost of allowing consumers to do whatever they want 😅

So I guess I'm asking what are the possible configurations we want to allow here?

Options

1. We only allow the label to be changed, this should be done with a parameter, we just replace the default aria-label on the Spinner with the custom text.
2. We have two parameters, one for a label, and one for an additional description. We would probably want to have the Spinner visible to the screen reader and add an aria-describedby pointing to the description (when it's provided). The description would need to be visible displayed as well. We'd then want to hide the visible description from the screen reader so that it's only spoken as context for the Spinner.
3. We have a slot which allows the user to make changes as they see fit (what we're doing here).

I'm going to go with (2) unless someone shouts and says "woah there Owen" 😂

There's an additional sub-option for (2) where we don't allow the label to be customised. I have no firm view on this but figure I'll do the most complex version and then we can remove the bits we don't want after discussion.

Slight aside: I couldn't think of a good way to test this so if you know a (good) way to short-circuit the loading process so that the Spinner remains visible indefinitely, let me know 🙂

[owenniblock]

OK, just need to find a good way to test the code 🤔

[camertron]

Just asking for some comments, but otherwise ready to merge! 🎉