SelectPanel: Add variant=modal #5817

siddharthkp · 2025-03-24T22:58:46Z

Note

Current status: main is failing integration tests, so need to run integration tests again once that is cleared up

Note: This PR does not include radio icons for single selection in a modal

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

Features:

Adding variant=modal does 3 things

Changes the position to be in the center of the screen
Makes onCancel a required prop
Adds a backdrop, when you click the backdrop (click-outside), we call onCancel

Shortcut

We are still using AnchoredOverlay underneath. But, for variant=modal, we override its position by directly passing top, left, etc. to overlayProps going down to the Overlay

The ideal super clean solution would have been to refactor SelectPanel and conditionally use useAnchoredPosition instead of AnchoredOverlay for variant=anchored. Something I did attempt in #5230 earlier but had to revert.

This shortcut is easier to implement and touches less surface area.

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2025-03-24T22:58:50Z

🦋 Changeset detected

Latest commit: d9640e3

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

This PR includes changesets to release 1 package

Name	Type
@primer/react	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 · 2025-03-24T22:59:20Z

👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks!

packages/react/src/Overlay/Overlay.tsx

packages/react/src/SelectPanel/SelectPanel.tsx

siddharthkp · 2025-03-26T22:07:14Z

packages/react/src/Overlay/Overlay.docs.json

@@ -143,6 +143,13 @@
    {
      "name": "sx",
      "type": "SystemStyleObject"
+    },
+    {
+      "name": "data-responsive",


Note for reviewer: I'm basically opening up an API to make Overlay responsive (fullscreen on narrow screens)

Should we call this responsiveVariant instead to make it feel more official? Or keep it subtle for now?

It only supports one value "fullscreen" and Overlay does not have variant so it does follow our object variant pattern 🤔 { narrow: 'fullscreen', regular: 'auto', wide: 'auto' } feels a bit odd.

Especially because Overlay is usually used by another component that passes it position props like top and left like AnchoredOverlay

I don't feel strongly either way, I don't see why not make it public though 👀

siddharthkp · 2025-03-26T22:08:14Z

packages/react/src/Overlay/Overlay.tsx

@@ -103,15 +103,6 @@ const StyledOverlay = toggleStyledComponent(
      max-width: calc(100vw - 2rem);
    }

-    &:where([data-variant='fullscreen']) {


Note for reviewer: Overlay's css module is in GA, so we don't need this css in sx anymore

I think let's keep this for now just in case, in theory the ga FF could still be turned off at any moment? The team can remove this when they remove the FF entirely from this component 🙏

I thought we weren't adding adding css for new features? Based on #5761 (comment)

I recently added this css in #5759, so thought I'll clean it up as well. yay/nay?

yeah I think the issue here is the styles won't be added at all because if FF is not enabled the Overlay classname will not be added so these styles will never make it in (why you need that FF on that other test), the correct way of adding new features without sx styling is to always have it go to css modules, for example if you created a separate class and added it to both (enabled, not enabled) implementations

github-actions · 2025-03-26T22:10:18Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	105.49 KB (+0.14% 🔺)
packages/react/dist/browser.umd.js	105.82 KB (+0.13% 🔺)

siddharthkp · 2025-03-27T19:53:28Z

packages/react/src/Overlay/Overlay.module.css

-    margin: 0;
-    border-radius: unset;
+  &:where([data-responsive='fullscreen']) {
+    @media screen and (--viewportRange-narrow) {


Note for reviewer: I changed full screen to only be a responsive thing instead of variant you can apply whenever. data-variant → data-responsive

packages/react/src/SelectPanel/SelectPanel.tsx

siddharthkp · 2025-03-27T19:57:32Z

packages/react/src/SelectPanel/SelectPanel.tsx

@@ -459,6 +464,16 @@ export function SelectPanel({
          role: 'dialog',
          'aria-labelledby': titleId,
          'aria-describedby': subtitle ? subtitleId : undefined,
+          ...(variant === 'modal'
+            ? {
+                /* override AnchoredOverlay position */


Note for reviewer: This is where all the shame lies. We are still using an AnchoredOverlay, but then we override it's position by directly passing top, left, etc to overlayProps going down

francinelucca

Mostly LGTM, just some questions/suggestions.

Was also wondering if you could elaborate on the feasibility of #5230? I see from the comments it was reverted due to failing integration checks, wondering how significant those were to warrant going this route. I'm ok with this solution, was just curious

e2e/components/SelectPanel.test.ts

francinelucca · 2025-03-28T04:13:53Z

packages/react/src/Overlay/Overlay.tsx

@@ -103,15 +103,6 @@ const StyledOverlay = toggleStyledComponent(
      max-width: calc(100vw - 2rem);
    }

-    &:where([data-variant='fullscreen']) {


I think let's keep this for now just in case, in theory the ga FF could still be turned off at any moment? The team can remove this when they remove the FF entirely from this component 🙏

francinelucca · 2025-03-28T04:18:26Z

packages/react/src/Overlay/Overlay.docs.json

@@ -143,6 +143,13 @@
    {
      "name": "sx",
      "type": "SystemStyleObject"
+    },
+    {
+      "name": "data-responsive",


I don't feel strongly either way, I don't see why not make it public though 👀

francinelucca · 2025-03-28T04:21:14Z

packages/react/src/SelectPanel/SelectPanel.tsx

@@ -459,6 +464,16 @@ export function SelectPanel({
          role: 'dialog',
          'aria-labelledby': titleId,
          'aria-describedby': subtitle ? subtitleId : undefined,
+          ...(variant === 'modal'
+            ? {
+                /* override AnchoredOverlay position */


siddharthkp · 2025-03-28T15:15:58Z

Was also wondering if you could elaborate on the feasibility of #5230? I see from the comments it was reverted due to failing integration checks, wondering how significant those were to warrant going this route. I'm ok with this solution, was just curious

It's definitely still feasible, but doesn't feel worth the effort anymore 🤔 I don't mind if the current solution stays long term too

@siddharthkp FYI these CI failures have nothing to do with this PR , #5802 (comment)

Thank you! I was about to debug

emilybrick

Hey! This looks good. Two minor things:

I think we should include the x icon here that does the same thing that onCancel would. This has always been the case in Figma. I know it may seem redundant, but I think the intent here is that it's expected behavior to have a close button in the top right of a modal dialog – in addition to the Cancel button in this case.

expected:

I discussed & confirmed w/ Primer design that the list items in the SelectPanel modal should use radios for single selection. Context

siddharthkp · 2025-03-28T20:15:18Z

@emilybrick, Oops i only added the close icon button to the css module (feature flagged), not without it. Added it to both now!

siddharthkp · 2025-03-28T20:47:36Z

packages/react/src/SelectPanel/SelectPanel.tsx

-      <Box
-        sx={enabled ? undefined : {display: 'flex', flexDirection: 'column', height: 'inherit', maxHeight: 'inherit'}}
-        className={enabled ? classes.Wrapper : undefined}
+    <>


Note for reviewer: Recommend to review this with hide whitespace

primer-integration · 2025-03-28T20:51:49Z

👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/370114

primer-integration · 2025-03-28T21:10:21Z

🔴 golden-jobs completed with status failure.

wip

fbd625c

siddharthkp self-assigned this Mar 24, 2025

github-actions bot added staff Author is a staff member integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm labels Mar 24, 2025

siddharthkp deployed to github-pages March 24, 2025 23:12 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5817 March 24, 2025 23:12 Inactive

siddharthkp commented Mar 25, 2025

View reviewed changes

packages/react/src/Overlay/Overlay.tsx Outdated Show resolved Hide resolved

siddharthkp commented Mar 25, 2025

View reviewed changes

packages/react/src/SelectPanel/SelectPanel.tsx Outdated Show resolved Hide resolved

call onCancel before OnOpenChange for modal + click outside

1b75223

siddharthkp deployed to github-pages March 26, 2025 21:29 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5817 March 26, 2025 21:30 Inactive

data-variant → data-responsive

cc94b85

siddharthkp commented Mar 26, 2025

View reviewed changes

disable anchor animation

be1a38a

github-actions bot temporarily deployed to storybook-preview-5817 March 26, 2025 22:20 Inactive

update snapshot

9e3f6de

github-actions bot requested a deployment to storybook-preview-5817 March 26, 2025 22:25 Abandoned

siddharthkp deployed to github-pages March 26, 2025 22:38 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5817 March 26, 2025 22:39 Inactive

siddharthkp added 2 commits March 27, 2025 20:46

changeset

ddd5d7d

add e2e test

1f73c86

siddharthkp added the update snapshots label Mar 27, 2025

siddharthkp commented Mar 27, 2025

View reviewed changes

packages/react/src/SelectPanel/SelectPanel.tsx Outdated Show resolved Hide resolved

siddharthkp commented Mar 27, 2025

View reviewed changes

test(vrt): update snapshots

0cd98d7

francinelucca added the integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh label Mar 28, 2025

github-actions bot added integration-tests: failing Changes in this PR cause breaking changes in gh/gh and removed integration-tests: passing Changes in this PR do NOT cause breaking changes in gh/gh labels Mar 28, 2025

francinelucca requested changes Mar 28, 2025

View reviewed changes

francinelucca requested a review from emilybrick March 28, 2025 04:26

emilybrick reviewed Mar 28, 2025

View reviewed changes

forgot to show x icon in the sx prop

ab31fb9

siddharthkp added 4 commits March 28, 2025 21:21

data-responsive → responsiveVariant

0169cf0

bring back the css + add media query

15f98a3

Merge branch 'main' into selectpanel-modal

6e34ef4

reply changes back on top of main

054f0eb

siddharthkp commented Mar 28, 2025

View reviewed changes

update jest snapshot

1a336ed

github-actions bot requested a deployment to storybook-preview-5817 March 28, 2025 20:54 Abandoned

siddharthkp deployed to github-pages March 28, 2025 21:07 — with GitHub Actions View deployment

github-actions bot temporarily deployed to storybook-preview-5817 March 28, 2025 21:07 Inactive

siddharthkp added the update snapshots label Mar 28, 2025

siddharthkp marked this pull request as ready for review March 28, 2025 21:50

siddharthkp requested review from a team as code owners March 28, 2025 21:50

siddharthkp requested review from maximedegreve and TylerJDev March 28, 2025 21:50

test(vrt): update snapshots

d9640e3

github-actions bot removed the update snapshots label Mar 28, 2025

primer bot deployed to github-pages March 28, 2025 22:13 View deployment

github-actions bot deployed to storybook-preview-5817 March 28, 2025 22:13 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SelectPanel: Add variant=modal #5817

SelectPanel: Add variant=modal #5817

siddharthkp commented Mar 24, 2025 •

edited

Loading

changeset-bot bot commented Mar 24, 2025 •

edited

Loading

github-actions bot commented Mar 24, 2025

siddharthkp Mar 26, 2025 •

edited

Loading

francinelucca Mar 28, 2025

siddharthkp Mar 26, 2025 •

edited

Loading

francinelucca Mar 28, 2025

siddharthkp Mar 28, 2025

francinelucca Mar 28, 2025

github-actions bot commented Mar 26, 2025 •

edited

Loading

siddharthkp Mar 27, 2025 •

edited

Loading

siddharthkp Mar 27, 2025

francinelucca Mar 28, 2025

francinelucca left a comment

francinelucca Mar 28, 2025

francinelucca Mar 28, 2025

francinelucca Mar 28, 2025

siddharthkp commented Mar 28, 2025 •

edited

Loading

emilybrick left a comment

siddharthkp commented Mar 28, 2025 •

edited

Loading

siddharthkp Mar 28, 2025

primer-integration bot commented Mar 28, 2025

primer-integration bot commented Mar 28, 2025

SelectPanel: Add variant=modal #5817

Are you sure you want to change the base?

SelectPanel: Add variant=modal #5817

Conversation

siddharthkp commented Mar 24, 2025 • edited Loading

Features:

Shortcut

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Mar 24, 2025 • edited Loading

🦋 Changeset detected

github-actions bot commented Mar 24, 2025

siddharthkp Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 26, 2025 • edited Loading

size-limit report 📦

siddharthkp Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francinelucca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp commented Mar 28, 2025 • edited Loading

emilybrick left a comment

Choose a reason for hiding this comment

siddharthkp commented Mar 28, 2025 • edited Loading

Choose a reason for hiding this comment

primer-integration bot commented Mar 28, 2025

primer-integration bot commented Mar 28, 2025

siddharthkp commented Mar 24, 2025 •

edited

Loading

changeset-bot bot commented Mar 24, 2025 •

edited

Loading

siddharthkp Mar 26, 2025 •

edited

Loading

siddharthkp Mar 26, 2025 •

edited

Loading

github-actions bot commented Mar 26, 2025 •

edited

Loading

siddharthkp Mar 27, 2025 •

edited

Loading

siddharthkp commented Mar 28, 2025 •

edited

Loading

siddharthkp commented Mar 28, 2025 •

edited

Loading