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.

Sign up for GitHub

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

Jump to bottom

Create <SegmentedPicker />, <Button />, <ButtonGroup /> components #1516

Merged
merged 22 commits into from
Jul 23, 2024
Merged

Create <SegmentedPicker />, <Button />, <ButtonGroup /> components #1516

merged 22 commits into from
Jul 23, 2024

Conversation

jessepinho
Copy link
Contributor

@jessepinho jessepinho commented Jul 18, 2024 •
edited
Loading

Originally was going to split out <SegmentedPicker /> and <Button* /> to separate PRs, but it turned out that they used shared styles, so I'm PRing them together.

In this PR

  • Create a <SegmentedPicker /> component
  • Create a <Button /> component
  • Create a <ButtonGroup /> component
  • Create a set of shared style utilities that takes care of hover/active/etc. states
  • Create Storybook stories for them all, and write some tests

Closes #115

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 5480a36

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

This PR includes changesets to release 3 packages
Name Type
@repo/ui Minor
minifront Patch
node-status 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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 18, 2024 •
edited
Loading

Visit the preview URL for this PR (updated for commit 5480a36):

https://penumbra-ui-preview--pr1516-jessepinho-segmented-d5zkeqe2.web.app

(expires Tue, 30 Jul 2024 18:58:37 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 709d729610ef7a6369b23f1cb2b820a60cc685b1

@jessepinho jessepinho changed the title Create <SegmentedPicker /> component WIP: Create <SegmentedPicker /> component Jul 18, 2024
@jessepinho jessepinho changed the title WIP: Create <SegmentedPicker /> component WIP: Create <SegmentedPicker />, <Button />, <ButtonGroup /> components Jul 19, 2024
jessepinho
jessepinho commented Jul 19, 2024
View reviewed changes
packages/ui/src/Button/index.stories.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://penumbra-ui-preview--pr1516-jessepinho-segmented-d5zkeqe2.web.app/?path=/docs/ui-library-button--docs

jessepinho
jessepinho commented Jul 19, 2024
View reviewed changes
packages/ui/src/ButtonGroup/index.stories.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://penumbra-ui-preview--pr1516-jessepinho-segmented-d5zkeqe2.web.app/?path=/docs/ui-library-buttongroup--docs

jessepinho
jessepinho commented Jul 19, 2024
View reviewed changes
packages/ui/src/SegmentedPicker/index.stories.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://penumbra-ui-preview--pr1516-jessepinho-segmented-d5zkeqe2.web.app/?path=/docs/ui-library-segmentedpicker--docs

@jessepinho jessepinho force-pushed the jessepinho/segmented-picker branch from 122abba to 5147459 Compare July 20, 2024 02:52
Base automatically changed from jessepinho/new-components-1 to jessepinho/sc-theme July 21, 2024 20:58
@jessepinho jessepinho force-pushed the jessepinho/segmented-picker branch from 5147459 to 3b0325e Compare July 21, 2024 21:06
Base automatically changed from jessepinho/sc-theme to main July 21, 2024 21:34
@jessepinho jessepinho force-pushed the jessepinho/segmented-picker branch from 3b0325e to c9dd66b Compare July 22, 2024 17:16
@jessepinho jessepinho changed the title WIP: Create <SegmentedPicker />, <Button />, <ButtonGroup /> components Create <SegmentedPicker />, <Button />, <ButtonGroup /> components Jul 22, 2024
@jessepinho jessepinho marked this pull request as ready for review July 22, 2024 17:34
@jessepinho jessepinho requested a review from a team July 22, 2024 17:34
@jessepinho
Copy link
Contributor Author

Moving back to draft to address some changes that came up in my recent 1:1 with Sam

@jessepinho jessepinho marked this pull request as draft July 22, 2024 21:41
@jessepinho jessepinho removed the request for review from a team July 22, 2024 21:41
@jessepinho
Copy link
Contributor Author

Just was one change after all. This PR is ready for review again 👍🏻

@jessepinho jessepinho marked this pull request as ready for review July 22, 2024 22:08
@jessepinho jessepinho requested a review from a team July 22, 2024 22:08
VanishMax
VanishMax reviewed Jul 23, 2024
View reviewed changes
packages/ui/src/Button/index.tsx Show resolved Hide resolved
packages/ui/src/Button/index.tsx Show resolved Hide resolved
packages/ui/src/SegmentedPicker/index.tsx Show resolved Hide resolved
packages/ui/src/SegmentedPicker/index.tsx Show resolved Hide resolved
packages/ui/src/SegmentedPicker/index.tsx
return (
<Root>
{options.map(option => (
<SegmentButton
Copy link
Contributor

Choose a reason for hiding this comment

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

thought/issue (a11y): semantically, SegmentedPicker is used as either a radio button group, or as tabs. Each of these usages have their W3C standards for accessibility. For now, the component doesn't fill the standards fully:

  1. Tabs: https://www.w3.org/WAI/ARIA/apg/patterns/tabs/examples/tabs-automatic/. SegmentedPicker doesn't support arrow navigation and aria attributes like aria-selected for the selected button, etc.
  2. Radio group: https://www.w3.org/WAI/ARIA/apg/patterns/radio/. SegmentedPicker doesn't use the required roles and keyboard navigation.

I don't mind having this version of a component for now – it looks good and works correctly, at least partially accessible with the keyboard. Yet, I believe that in the future SegmentedPicker should be split into RadioGroup and Tabs components correspondingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is a good call-out — this seems like a great candidate for using an already existing component (like Radix UI's Tabs). Since we're going to be using <SegmentedPicker /> for tabs anyway, that seems like the right fit. I'll move this PR back to draft until this is addressed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, on second thought, I'll do this as a separate PR to keep PRs smaller.

packages/ui/src/ButtonPriorityContext.ts Outdated Show resolved Hide resolved
packages/ui/src/utils/button.ts Show resolved Hide resolved
@jessepinho jessepinho mentioned this pull request Jul 23, 2024
Closed
VanishMax
VanishMax approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@VanishMax VanishMax left a comment

Choose a reason for hiding this comment

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

Ready to merge!

@jessepinho jessepinho merged commit 54a5d66 into main Jul 23, 2024
8 checks passed
@jessepinho jessepinho deleted the jessepinho/segmented-picker branch July 23, 2024 21:27
@jessepinho jessepinho mentioned this pull request Jul 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VanishMax VanishMax VanishMax approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jessepinho @VanishMax