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.

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

Upstream the SelectPanel component from dotcom #2941

Merged
merged 22 commits into from
Jul 19, 2024
Merged

Conversation

camertron
Copy link
Contributor

@camertron camertron commented Jul 9, 2024

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?

This PR upstreams the existing Primer::Experimental::SelectPanel component from dotcom to PVC. SelectPanel is a component that we've been incubating in dotcom for several months now, and has helped us remediate a number of accessibility issues. The component has reached the right level of maturity to bring it to PVC.

I also expanded our @snapshot interactive functionality to support waiting for SelectPanel items to load before taking a screenshot. This is accomplished by adding a data-interaction-subject attribute to the <select-panel> element and injecting a bit of JavaScript to preview pages. The JavaScript waits for items to load by listening for the newly-added loadend event, which subsequently adds a data-ready="true" attribute. The snapshotting code then waits for an element with [data-ready="true"] to appear before taking a screenshot.

Screenshots

A screenshot of an open SelectPanel, rendered below the button that opened it, showing 4 items.

Integration

As part of upstreaming, I had to remove a reference to a dotcom-specific helper that returned the URL for GitHub's support portal. In the process I realized the two error messages the component displays were not customizable, so I added the preload_error_content and error_content slots. The former is displayed in place of the list when the panel first opens and fetches the initial set of list items. The latter renders as a banner after a successful fetch of the initial list items when the list is filtered.

The component provides default content for these slots, but we will need to update usages in dotcom to maintain existing behavior, i.e. maintain a link to the support portal in error messages.

This PR also introduces the Primer::Alpha::SelectPanel::ItemList class, which is currently just an alias for Primer::Alpha::ActionList. At the moment, consumers are expected to know (perhaps from reading our docs) that they're supposed to render an ActionList for the list of items. Unfortunately we've seen some confusion around this, with some Hubbers rendering ActionMenu::Lists etc that appear to work but that can lead to subtle bugs. Introducing a class scoped explicitly to SelectPanel should help make the right choice more obvious. While upgrading to ItemList in dotcom is not strictly required (since ItemList is an alias of ActionList), it is recommended to avoid confusion and to give us a place to make changes in the future.

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.

Accessibility

  • No new axe scan violation - This change does not introduce any new axe scan violations.

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.

Copy link

changeset-bot bot commented Jul 9, 2024

🦋 Changeset detected

Latest commit: 6f1e31e

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

Copy link
Contributor

github-actions bot commented Jul 11, 2024

⚠️ 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 camertron requested review from mperrotti, jonrohan and kendallgassner and removed request for mperrotti July 12, 2024 20:32
Copy link
Contributor

@kendallgassner kendallgassner left a comment

Choose a reason for hiding this comment

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

Only concern is the aria-live library! I would love to use the Primer live-region-element since we are moving to it anyway!

Copy link
Contributor

@kendallgassner kendallgassner left a comment

Choose a reason for hiding this comment

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

Thank you 🌻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants