Connection: Step survey - modernize components and fix for accessibility - #50776
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 3 files.
|
There was a problem hiding this comment.
Pull request overview
Modernizes the disconnect step survey in the Connection UI by replacing custom “button-like div” option cards with a native radio group implementation, improving keyboard and assistive-technology behavior and aligning UI primitives with @wordpress/ui.
Changes:
- Rebuild survey options as a single named radio group (fieldset/legend + native radios) and refactor
SurveyChoiceto render@wordpress/uiCard+ radio/label. - Fix survey state handling (initialize custom response to
'', makeonSubmitoptional, and guard submit when no option is selected). - Add Jest coverage for DisconnectSurvey and StepSurvey behaviors (group semantics, keyboard selection, submission).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/connection/components/disconnect-survey/types.ts | Updates SurveyChoiceProps to match the new radio-based option card API. |
| projects/js-packages/connection/components/disconnect-survey/test/index.tsx | Adds tests covering radio grouping, keyboard interaction, submit behavior, and “Other” free-text selection. |
| projects/js-packages/connection/components/disconnect-survey/survey-choice.tsx | Refactors option rendering to a Card containing a native radio + label, removing custom key handling. |
| projects/js-packages/connection/components/disconnect-survey/index.tsx | Implements per-instance radio group naming via useId(), fieldset/legend labeling, optional submit handler, and improved “Other” handling. |
| projects/js-packages/connection/components/disconnect-survey/_jp-connect_disconnect-survey-card.scss | Updates styling for the new Card/radio layout and fixes the selected hover selector. |
| projects/js-packages/connection/components/disconnect-dialog/test/step-survey.tsx | Adds StepSurvey tests validating heading/copy, radio rendering, and callback behavior. |
| projects/js-packages/connection/components/disconnect-dialog/style.scss | Adjusts spacing/layout for large text and the new fieldset-based survey container. |
| projects/js-packages/connection/components/disconnect-dialog/steps/step-survey.tsx | Switches intro copy to Text and tightens the survey callback typing. |
| projects/js-packages/connection/changelog/update-connection-js-package-ui-pt6 | Adds a changelog entry describing the survey accessibility/UI modernization. |
bindlegirl
left a comment
There was a problem hiding this comment.
Looks good and works well.
Fixes CONNECT-393
Ideally requires #50735 as there is some overlap with changes in
step-survey.tsx.Proposed changes
survey-choice.tsxrenders each option as a focusabledivwithrole="button". This resulted in several accessibility issues: Selected state was not exposed correctly to assistive technology, keyboard behaviour did not match a single-choice control, focus was managed by hand on a non-semantic element, interactive content was nested inside an element withrole="button", and the control behaved as a radio group but was not implemented as one.Now:
surveyGroupName, generated per instance withuseId()), so the browser supplies rovingtabindex, arrow-key navigation, and one tab stop for the whole group.onKeyDownhandler deleted — native behaviour replaces it.role="button"; it's a sibling of the<label>, so no interactive-in-label nesting.divis gone with thediv.<fieldset>+VisuallyHidden render={<legend/>}naming the group.label::before { inset: 0 }; free-text field raised withz-index: 1so clicks land on it, and interacting with it (click or typing) selects "Other" — matching old click-to-select.Shared card component:
Cardfrom@wordpress/ui, matchingmanage-connection-action-cardin the connection dialogs. Card is presentational only and contributes nothing to the accessibility above — the radio inside it carries the semantics.Additional changes:
:not(.jp-disconnect-survey-card--selected)— missingconnect__, so it never matched and the hover border overrode the selected border.step-survey.tsxlarge-text<p>now using<Text>customResponsedefault - it was undefined, resulting in a console error when beginning to type. Now an empty string.DisconnectSurvey'sonSubmitprop is now optional and invoked with optional chaining (onSubmit?.()). The prop was previously declared as required, butStepSurveyforwards its own optionalonFeedBackProvidedprop into it — so an undefined callback could reach an unguarded call site and throw at runtime.handleSurveySubmitnow returns early when no answer is selected. The submit button is already disabled until the user picks an option, so this is a guard rather than a behaviour change, but it narrowsselectedAnswerfromstring | undefinedtostring. The survey'sonSubmitandStepSurvey'sonFeedBackProvidedsignatures have been tightened to match, bringing all three layers in line withhandleSubmitSurveyinDisconnectDialog, which already declared the answer ID as a non-optionalstring.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
On a Jetpack-connected site with this PR applied (locally or using the Jetpack Beta tester plugin):
setIsProvidingFeedback( true );withinhandleDisconnectinDisconnectDialogto get this to work, so testing locally and re-building for those changes (rebuild My Jetpack and dependencies) is what worked for me.To test that the survey still renders properly via the Jetpack plugin:
wp-admin/admin.php?page=jetpack#/disconnect.Before:
After: