Forms: Fix aria-label attribute by passing string values to Page #47481
Forms: Fix aria-label attribute by passing string values to Page #47481
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! |
There was a problem hiding this comment.
Pull request overview
This PR updates the Jetpack Forms wp-build dashboards to provide a proper string aria-label for the @wordpress/admin-ui Page navigable region when the visible title is a React element (e.g., logo + text), preventing the DOM from ending up with an aria-label like [object Object].
Changes:
- Add an
ariaLabel: stringfield tousePageHeaderDetails()and compute an appropriate label for list vs single-form screens. - Pass the new
ariaLabelvalue into thePagecomponent in both Forms and Responses wp-build routes. - Add a Forms package changelogger entry documenting the fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| projects/packages/forms/src/dashboard/wp-build/hooks/use-page-header-details.tsx | Adds computed ariaLabel string to the header details returned by the hook. |
| projects/packages/forms/routes/responses/stage.tsx | Wires ariaLabel from the hook into the wp-build <Page /> usage on the Responses route. |
| projects/packages/forms/routes/forms/stage.tsx | Wires ariaLabel from the hook into the wp-build <Page /> usage on the Forms route. |
| projects/packages/forms/changelog/update-forms-aria-label | Adds changelogger entry for the accessibility fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // "Forms" is a product name, do not translate. | ||
| return 'Jetpack Forms'; | ||
| }, [ isSingleFormScreen, formTitle ] ); |
There was a problem hiding this comment.
ariaLabel for non-single-form screens returns 'Jetpack Forms', but the visible header title is the product name Forms (and the comment says "Forms" is the product name). This creates an inconsistency and also doesn’t match the PR/testing note that the landmark label should become Forms. Consider using the same string as the displayed product name for ariaLabel (and/or centralize it as a shared constant) so the landmark label matches the UI.
There was a problem hiding this comment.
Visible title is Jetpack logo + Forms, hence "Jetpack Forms".
Related to JETPACK-1400
Follow up to #47461 to cover Forms, as previous PR covered everything else.
Proposed changes:
ariaLabelto Page since we're using React components intitleand aria-label requires string.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
[object object]toJetpack FormsChangelog