-
Notifications
You must be signed in to change notification settings - Fork 2k
DS Site: Add @wordpress/components reference
#103738
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
|
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
@wordpress/components reference@wordpress/components reference
# Conflicts: # apps/design-system-docs/package.json
|
Design System Reference Site Preview: https://bf3ad454-preview.a8cds.workers.dev (Latest commit: 998308b) |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please excuse the raw markdown output for the notes column in this MVP 😇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: I similarly needed to format raw Markdown in #103561 and opted to include react-markdown as used elsewhere in the project. Can be a future item.
aduth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍 LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: I similarly needed to format raw Markdown in #103561 and opted to include react-markdown as used elsewhere in the project. Can be a future item.
| if ( ! href ) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: IMO, it'd make more sense to have WPComponentsTable avoid rendering IconLink if the link is empty, rather than handling that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 5280683 👍
| return ( | ||
| <Tooltip text={ statuses.find( ( s ) => s.value === status )?.label }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're using the corresponding statuses value 3 times, maybe we ought to assign a variable?
| return ( | |
| <Tooltip text={ statuses.find( ( s ) => s.value === status )?.label }> | |
| const { label, icon } = statuses.find( ( s ) => s.value === status )!; | |
| return ( | |
| <Tooltip text={ label }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed in 998308b.
| <Tooltip text={ statuses.find( ( s ) => s.value === status )?.label }> | ||
| <div | ||
| className={ styles[ 'status-indicator' ] } | ||
| aria-label={ statuses.find( ( s ) => s.value === status )?.label } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to have both text= and aria-label= ? Is the Tooltip not already handling the accessible name / description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question. Short answer is yes, we need both here because Tooltip only applies an accessible description on focus, and screen readers can read this icon without it being focused.
The long answer is more interesting. At some point we discovered that a tooltip probably shouldn't be adding an accessible string by default. That was reported up to Ariakit, and now Ariakit doesn't do that anymore (ariakit/ariakit#3242). We haven't been able to propagate those new defaults to Gutenberg itself yet though, due to back compat (WordPress/gutenberg#66021).
Closes DS-236
Proposed Changes
Adds the package reference for
@wordpress/componentsto the DS site.Based on the component audit we did as a group in late 2024, I updated and verified all the links and information, and edited the content to be relevant to consumers from A8C.
Why are these changes being made?
To provide basic usage guidance on
@wordpress/componentscomponents as we work on the new components library.Testing Instructions
yarn && cd apps/design-system-docsandyarn start.Pre-merge Checklist