Skip to content

Conversation

@siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Feb 20, 2024

@changeset-bot
Copy link

changeset-bot bot commented Feb 20, 2024

🦋 Changeset detected

Latest commit: 065f107

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

@siddharthkp siddharthkp self-assigned this Feb 20, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Feb 20, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 113.71 KB (+0.02% 🔺)
packages/react/dist/browser.umd.js 114.38 KB (+0.01% 🔺)

@github-actions github-actions bot temporarily deployed to storybook-preview-4288 February 20, 2024 12:08 Inactive
@siddharthkp siddharthkp marked this pull request as ready for review February 20, 2024 12:17
@siddharthkp siddharthkp requested a review from a team February 20, 2024 12:17
@joshblack
Copy link
Member

@siddharthkp for reviewing, should I be looking at the semantics of the nested selection story or should I be just looking at the back button behavior? Seeing the initial listbox was surprising so just wanted to double-check what I should be looking at in this PR 👀

@siddharthkp
Copy link
Member Author

@siddharthkp for reviewing, should I be looking at the semantics of the nested selection story or should I be just looking at the back button behavior?

Both are welcome!

Seeing the initial listbox was surprising

I can go either way on this 🤔

Repositories from the first panel still could be treated as "selected", with PRs inside them being displayed outside the panel.

Orrr we could treat the first panel/layer simply as filtering buttons that lead to the actual "options" that can be selected.

What do you think?

Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Going through this, something I'm not sure about is the listbox being used for nested selection in this way. Something that is disorienting is that you make a selection in a listbox and then shift to a text input and you don't necessarily know why depending on the screen reader. This kind of relationship almost feels more like checkbox groups or a tree.

When going through it with voiceover, I noticed that sometimes the virtual cursor and the focused element got out of sync in selection 🤔 Not sure why but this may be unrelated to this PR. It also seems like hitting escape out of the second dialog will not restore focus to the button and will make it so that you cannot open the dialog again.

@siddharthkp
Copy link
Member Author

Going through this, something I'm not sure about is the listbox being used for nested selection in this way. Something that is disorienting is that you make a selection in a listbox and then shift to a text input and you don't necessarily know why depending on the screen reader.

Very valid!

I changed it to role=list for now and added a warning about accessibility on the story. Let's focus on shipping the back button and then we can get some guidance on the accessible markup for this story!

Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

Approving for onBack support 👍

})

const SelectPanelHeader: React.FC<React.PropsWithChildren> = ({children, ...props}) => {
const SelectPanelHeader: React.FC<React.PropsWithChildren & {onBack?: () => void}> = ({children, onBack, ...props}) => {
Copy link
Member

Choose a reason for hiding this comment

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

Another syntax if it's helpful, React.PropsWithChildren is generic over input props

Suggested change
const SelectPanelHeader: React.FC<React.PropsWithChildren & {onBack?: () => void}> = ({children, onBack, ...props}) => {
const SelectPanelHeader: React.FC<React.PropsWithChildren<{onBack?: () => void}>> = ({children, onBack, ...props}) => {

@siddharthkp siddharthkp requested a review from a team as a code owner March 7, 2024 10:25
@siddharthkp siddharthkp requested a review from jonrohan March 7, 2024 10:25
@siddharthkp siddharthkp added this pull request to the merge queue Mar 7, 2024
Merged via the queue into main with commit 200fb18 Mar 7, 2024
@siddharthkp siddharthkp deleted the drafts-selectpanel-back-button branch March 7, 2024 10:46
@primer primer bot mentioned this pull request Mar 6, 2024
@siddharthkp siddharthkp changed the title experimental/SelectPanel2: Add back button SelectPanel2: Add back button Mar 19, 2024
lukasoppermann pushed a commit that referenced this pull request Apr 16, 2024
* add back button

* add nested panel example

* fix nested panel focus

* implement save and cancel on second panel

* add onCancel for first panel

* Create fair-sloths-kick.md

* add preview for selection

* show dividers

* hide selection in story

* update snapshots

* make the first ActionList role=list

* add warning to story
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants