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

feat(forms): add segmented radio field #83557

Merged
merged 5 commits into from
Jan 16, 2025
Merged

Conversation

natemoo-re
Copy link
Member

@natemoo-re natemoo-re commented Jan 15, 2025

Part of ACI M6.1.

  • Introduces a new segmented radio field (mostly copied from the existing radio field, with some styling changes)
  • fixes an accessibility issue with the existing radio field, which did not pass name down to the input. Without this change, keyboard navigation for multiple radio fields on the same page was broken.
  • Adds to form field story
segmented-radio

@natemoo-re natemoo-re requested a review from a team as a code owner January 15, 2025 23:32
@natemoo-re natemoo-re requested a review from a team January 15, 2025 23:32
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jan 15, 2025

return (
<Tooltip
key={index}
Copy link
Member

Choose a reason for hiding this comment

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

key could maybe be id but maybe we don't know that will be unique

Copy link
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

i think this generally makes sense, but we may want to decide on the react-hook-form vs components/Form before we do too many of these components to avoid any extra work.

import {space} from 'sentry/styles/space';

export interface SegmentedRadioFieldProps extends Omit<InputFieldProps, 'type'> {
choices?: RadioGroupProps<any>['choices'];
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 should this any propagate the generic type down instead?

Copy link
Member Author

@natemoo-re natemoo-re Jan 16, 2025

Choose a reason for hiding this comment

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

Ah great catch, missed this in the copy + paste. Fixed in 2889f50!

static/app/components/forms/fields/segmentedRadioField.tsx Outdated Show resolved Hide resolved
@scttcper
Copy link
Member

link to one of the recent vercel stories https://sentry-i5qt28rm9.sentry.dev/stories/?name=app%2Fcomponents%2Fforms%2Ffields%2Findex.stories.tsx&query=

Copy link

codecov bot commented Jan 16, 2025

Bundle Report

Changes will increase total bundle size by 3.78kB (0.01%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
app-webpack-bundle-array-push 31.3MB 3.78kB (0.01%) ⬆️

@natemoo-re natemoo-re force-pushed the aci/m61-segmented-radio branch from 9541fb8 to 2889f50 Compare January 16, 2025 20:03
@natemoo-re natemoo-re enabled auto-merge (squash) January 16, 2025 20:03
@natemoo-re natemoo-re merged commit c290a37 into master Jan 16, 2025
41 checks passed
@natemoo-re natemoo-re deleted the aci/m61-segmented-radio branch January 16, 2025 20:15
andrewshie-sentry pushed a commit that referenced this pull request Jan 22, 2025
Introduces a new segmented radio field (mostly copied from the
existing radio field, with some styling changes)
- fixes an accessibility issue with the existing radio field, which did
not pass `name` down to the `input`. Without this change, keyboard
navigation for multiple radio fields on the same page was broken.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants