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

Selectpanel2: Remove SelectPanel.ActionList API #3764

Merged
merged 43 commits into from
Oct 2, 2023

Conversation

siddharthkp
Copy link
Member

@siddharthkp siddharthkp commented Sep 25, 2023

Part 1: tiny API change

API before this PR:

<SelectPanel>
  ...
  <SelectPanel.ActionList>
    <ActionList.Item>
     <ActionList.LeadingVisual><Avatar/></ActionList.LeadingVisual>
      Armağan 
      <ActionList.Description>broccolinisoup</ActionList.Description>
    </ActionList.Item>
    <ActionList.Item>
     <ActionList.LeadingVisual><Avatar/></ActionList.LeadingVisual>
      Pavithra 
      <ActionList.Description>pksjce</ActionList.Description>
    </ActionList.Item>
  </SelectPanel.ActionList>
</SelectPanel>

Hard to guess that it's SelectPanel.ActionList but ActionList.Item, ActionList.LeadingVisual inside it etc.

We could recommend

  • a) SelectPanel.ActionList.Item and SelectPanel.Item.LeadingVisual
  • b) Or SelectPanel.Item and SelectPanel.LeadingVisual...

but, it feels even nicer if we don't need either. ActionList already supports a composition-friendly context for container components.

New API:

<SelectPanel>
  ...
- <SelectPanel.ActionList>
+ <ActionList>
    <ActionList.Item>
     <ActionList.LeadingVisual><Avatar/></ActionList.LeadingVisual>
      Armağan 
      <ActionList.Description>broccolinisoup</ActionList.Description>
    </ActionList.Item>
    <ActionList.Item>
     <ActionList.LeadingVisual><Avatar/></ActionList.LeadingVisual>
      Pavithra 
      <ActionList.Description>pksjce</ActionList.Description>
    </ActionList.Item>
- </SelectPanel.ActionList>
+ </ActionList>
</SelectPanel>

Part 2: it's free real estate accessibility attributes

Utilising the ActionListContainerContext, we can set role for ActionList from the container (SelectPanel) and adapt item roles within ActionList

Before After
accessibility dom tree, before, list is rendered with role=list and items as role=listitem accessibility dom tree, before, list is rendered with role=listbox and items as role=option

@changeset-bot

This comment was marked as resolved.

@github-actions

This comment was marked as resolved.

@siddharthkp siddharthkp added the skip changeset This change does not need a changelog label Sep 25, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 25, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 104.55 KB (+0.03% 🔺)
dist/browser.umd.js 105.13 KB (+0.03% 🔺)

@siddharthkp siddharthkp temporarily deployed to github-pages September 25, 2023 11:47 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3764 September 25, 2023 11:47 Inactive
{/* render default header as fallback */}
{slots.header || <SelectPanel.Header />}
{childrenInBody}
<Box
sx={{
Copy link
Member Author

Choose a reason for hiding this comment

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

moved the styles from SelectPanel.ActionList here

@siddharthkp siddharthkp temporarily deployed to github-pages September 26, 2023 19:01 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3764 September 26, 2023 19:01 Inactive
@siddharthkp siddharthkp changed the base branch from main to lol-sid September 26, 2023 19:13
Base automatically changed from lol-sid to main September 26, 2023 21:46
@siddharthkp siddharthkp temporarily deployed to github-pages September 27, 2023 12:28 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3764 September 27, 2023 12:29 Inactive
@siddharthkp siddharthkp marked this pull request as draft September 27, 2023 12:34
@siddharthkp siddharthkp temporarily deployed to github-pages September 27, 2023 12:39 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3764 September 27, 2023 12:39 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3764 September 27, 2023 20:19 Inactive
@siddharthkp siddharthkp temporarily deployed to github-pages September 27, 2023 20:25 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3764 September 27, 2023 20:26 Inactive
@siddharthkp siddharthkp marked this pull request as ready for review September 27, 2023 21:10
Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Just left a question and the API change makes sense to me 👍🏻

@@ -55,13 +54,13 @@ const SelectPanel = props => {
React.useEffect(() => setInternalOpen(props.open), [props.open])

const onInternalClose = () => {
if (props.open === 'undefined') setInternalOpen(false)
if (props.open === undefined) setInternalOpen(false)
Copy link
Member

Choose a reason for hiding this comment

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

Oops I didn't catch that in the previous PR too 😬

Comment on lines +128 to +129
listRole: 'listbox',
selectionAttribute: 'aria-selected',
Copy link
Member

Choose a reason for hiding this comment

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

From what I understand, the role is always going to be listbox and selectionAttribute be aria-selected for SelectPanel. is that right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, that is always true :)

@siddharthkp siddharthkp added this pull request to the merge queue Oct 2, 2023
Merged via the queue into main with commit 167d9f6 Oct 2, 2023
30 checks passed
@siddharthkp siddharthkp deleted the drafts-selectpanel-simplify-actionlist branch October 2, 2023 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changeset This change does not need a changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants