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

ActionList Fixes #1215

Merged
merged 18 commits into from
May 12, 2021
Merged

ActionList Fixes #1215

merged 18 commits into from
May 12, 2021

Conversation

dgreif
Copy link
Member

@dgreif dgreif commented May 7, 2021

Addressing a number of issues found when integrating ActionList into SelectPanel

  • Divider should be hidden above a group that has a filled header
  • inline description is not inline
  • Multi-select action lists should use checkboxes
    • Checkbox instead of checkmark icon
    • Properly align checkbox vertically
    • Wrap item in a <label>? (Using aria-label={text} to avoid branching DOM and for succinct announcements)
    • Prevent tabbing to checkbox if in focusZone (this one might be tough)
    • Update docs with new selectionVariant option (Prop is commented in the ItemProps interface definition, but ActionList.Item does not have a corresponding .mdx file)

Screenshots

Before After
image image
image image
image image

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • 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
Copy link

changeset-bot bot commented May 7, 2021

🦋 Changeset detected

Latest commit: 2d604c6

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

This PR includes changesets to release 1 package
Name Type
@primer/components Patch

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

@vercel
Copy link

vercel bot commented May 7, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/primer-components/AwfrL2RE61W7UMpqyhpBnACmCd7L
✅ Preview: https://primer-components-git-action-list-fixes-primer.vercel.app

@vercel vercel bot temporarily deployed to Preview May 10, 2021 16:02 Inactive
@vercel vercel bot temporarily deployed to Preview May 10, 2021 16:21 Inactive
@vercel vercel bot temporarily deployed to Preview May 10, 2021 20:00 Inactive
@vercel vercel bot temporarily deployed to Preview May 10, 2021 20:18 Inactive
@vercel vercel bot temporarily deployed to Preview May 10, 2021 21:13 Inactive
@vercel vercel bot temporarily deployed to Preview May 10, 2021 21:46 Inactive
@vercel vercel bot temporarily deployed to Preview May 11, 2021 15:22 Inactive
@vercel vercel bot temporarily deployed to Preview May 11, 2021 15:25 Inactive
@vercel vercel bot temporarily deployed to Preview May 11, 2021 21:22 Inactive
@dgreif dgreif marked this pull request as ready for review May 11, 2021 22:40
@vercel vercel bot temporarily deployed to Preview May 11, 2021 22:50 Inactive
@vercel vercel bot temporarily deployed to Preview May 11, 2021 22:54 Inactive
Copy link
Contributor

@VanAnderson VanAnderson left a comment

Choose a reason for hiding this comment

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

LGTM!

@vercel vercel bot temporarily deployed to Preview May 12, 2021 16:21 Inactive
Hide divider before `ActionList.Group`s with filled header
Align `Item` description to when rendered in-line
Add `selectionVariant: 'multiple'` for `Item`s. These will use a checkbox input instead of a checkmark icon for selected state
Allow `focusZoneSettings` to be passed into `AnchoredOverlay`
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's not too much trouble, it might be nice to put all these distinct changes in their own changeset.

src/ActionList/List.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Looks great, @dgreif! Thanks for cleaning this up ❤️

Just left a couple non-blocking comments.

@vercel vercel bot temporarily deployed to Preview May 12, 2021 18:46 Inactive
@vercel vercel bot temporarily deployed to Preview May 12, 2021 18:50 Inactive
dgreif and others added 5 commits May 12, 2021 11:50
… 'Dialog', to avoid clipping focus ring outlines (particularly in Safari). Because this would collapse margins, add 'position: absolute' to 'ErsatzOverlay' to avoid collapsing 'List'’s top and bottom margins
@vercel vercel bot temporarily deployed to Preview May 12, 2021 18:54 Inactive
@vercel vercel bot temporarily deployed to Preview May 12, 2021 19:44 Inactive
@dgreif dgreif merged commit c63fa4b into main May 12, 2021
@dgreif dgreif deleted the action-list-fixes branch May 12, 2021 19:53
@github-actions github-actions bot mentioned this pull request May 12, 2021
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.

None yet

4 participants