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

Button: Revise documentation #66617

Merged
merged 12 commits into from
Dec 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
327 changes: 108 additions & 219 deletions packages/components/src/button/README.md

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions packages/components/src/button/docs-manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"$schema": "../../schemas/docs-manifest.json",
"displayName": "Button",
"filePath": "./index.tsx"
}
31 changes: 31 additions & 0 deletions packages/components/src/button/stories/best-practices.mdx
Copy link
Member Author

Choose a reason for hiding this comment

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

How about we write the supplementary docs in a file like this? I'm having second thoughts about putting all the documentation into the main Docs page, because it can already be quite long with the props table and the stories.

Copy link
Contributor

Choose a reason for hiding this comment

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

Chat notes:

  • Add TOC to Storybook generated DOCS page, linking to props and best practices
  • Add "Accessibility" section
  • Add a "Related components" section at the bottom
  • Add section on how to write component's documentation in the contributing guidelines
  • [optional] add linter to enforce some of the guidelines above

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { Meta } from '@storybook/blocks';

<Meta title="Components/Actions/Button/Best Practices" />

# Button

## Usage
Buttons indicate available actions and allow user interaction within the interface. As key elements in the WordPress UI, they appear in toolbars, modals, and forms. Default buttons support most actions, while primary buttons emphasize the main action in a view. Secondary buttons pair as secondary actions next to a primary action.

Each layout contains one prominently placed, high-emphasis button. If you need multiple buttons, use one primary button for the main action, secondary for the rest of the actions and tertiary sparingly when an action needs to not stand out at all.

### Sizes

- `'default'`: For normal text-label buttons, unless it is a toggle button.
- `'compact'`: For toggle buttons, icon buttons, and buttons when used in context of either.
- `'small'`: For icon buttons associated with more advanced or auxiliary features.

## Best practices

- Label buttons to show that a click or tap initiates an action.
- Use established color conventions; for example, reserve red buttons for irreversible or dangerous actions.
- Avoid crowding the screen with multiple calls to action, which confuses users.
- Keep button locations consistent across the interface.

## Content guidelines

Buttons should be clear and predictable, showing users what will happen when clicked. Make labels reflect actions accurately to avoid confusion.

Start button text with a strong action verb and include a noun to specify the change, except for common actions like Save, Close, Cancel, or OK.

For other actions, use a `{verb}+{noun}` format for context. Keep button text brief and remove unnecessary words like "the," "an," or "a" for easy scanning.
18 changes: 18 additions & 0 deletions packages/components/src/button/stories/index.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,30 +65,48 @@ Default.args = {
children: 'Code is poetry',
};

/**
* Primary buttons stand out with bold color fills, making them distinct
* from the background. Since they naturally draw attention, each layout should contain
* only one primary button to guide users toward the most important action.
*/
Comment on lines +68 to +72
Copy link
Member Author

Choose a reason for hiding this comment

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

I am reevaluating this PR from a user perspective, and I realized that we're probably not adding much value by showing a separate page with variants, including a full specimen of hover/active/disabled states. Also, some of the states are already wrong (i.e. not the same as in shipping code). For example, look at all the Link variants — they're all wrong. @auareyou Can you troubleshoot this? Are we using the Figma library components incorrectly in some way?

What I see in the Figma embed

Figma embed for Link variant

Actual Link variant (default)

Link variant

We already show an interactive list of all the variants in the normal story docs, so I suggest we just move the copy there. That's where all this kind of basic, important information should live. What do you think?

Story with description

The Best Practices page is probably a good home for more supplementary information, like patterns/examples that don't quite fit in the normal story docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we using the Figma library components incorrectly in some way?

There's an issue in Figma where replacing underlined text layer values removes the underline 🙃 I fixed it.

I am reevaluating this PR from a user perspective, and I realized that we're probably not adding much value by showing a separate page with variants, including a full specimen of hover/active/disabled states.

I tend to agree with this though. The value doesn't quite reflect the effort, imo.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved all the variant descriptions to the main stories 👍

Figma embeds should be a last resort, given the slow load times and maintenance cost.

export const Primary = Template.bind( {} );
Primary.args = {
...Default.args,
variant: 'primary',
};

/**
* Secondary buttons complement primary buttons. Use them for standard actions that may appear alongside a primary action.
*/
export const Secondary = Template.bind( {} );
Secondary.args = {
...Default.args,
variant: 'secondary',
};

/**
* Tertiary buttons have minimal emphasis. Use them sparingly to subtly highlight an action.
*/
export const Tertiary = Template.bind( {} );
Tertiary.args = {
...Default.args,
variant: 'tertiary',
};

/**
* Link buttons have low emphasis and blend into the page, making them suitable for supplementary actions,
* especially those involving navigation away from the current view.
*/
export const Link = Template.bind( {} );
Link.args = {
...Default.args,
variant: 'link',
};

/**
* Use this variant for irreversible actions. Apply sparingly and only for actions with significant impact.
*/
export const IsDestructive = Template.bind( {} );
IsDestructive.args = {
...Default.args,
Expand Down
10 changes: 6 additions & 4 deletions packages/components/src/button/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,13 @@ type BaseButtonProps = {
tooltipPosition?: PopoverProps[ 'position' ];
/**
* Specifies the button's style.
*
* The accepted values are:
* 'primary' (the primary button styles)
* 'secondary' (the default button styles)
* 'tertiary' (the text-based button styles)
* 'link' (the link button styles)
*
* 1. `'primary'` (the primary button styles)
* 2. `'secondary'` (the default button styles)
* 3. `'tertiary'` (the text-based button styles)
* 4. `'link'` (the link button styles)
Comment on lines +117 to +120
Copy link
Member Author

Choose a reason for hiding this comment

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

These descriptions need to be improved (see size prop description for example). It also fails to capture that there is a default (variant={ undefined }) variant.

*/
variant?: 'primary' | 'secondary' | 'tertiary' | 'link';
};
Expand Down
47 changes: 47 additions & 0 deletions storybook/components/figma-embed/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* Internal dependencies
*/
import './style.scss';

// See https://www.figma.com/developers/embed#embed-a-figma-file
const CONFIG = {
'embed-host': 'wordpress-storybook',
footer: false,
'page-selector': false,
'viewport-controls': true,
};

/**
* Embed Figma links in the Storybook.
*
* @param {Object} props
* @param {string} props.url - Figma URL to embed.
* @param {string} props.title - Accessible title for the iframe.
*/
function FigmaEmbed( { url, title, ...props } ) {
const urlObj = new URL( url );

const queryParams = new URLSearchParams( urlObj.search );
Object.entries( CONFIG ).forEach( ( [ key, value ] ) => {
queryParams.set( key, value );
} );
urlObj.search = queryParams.toString();

urlObj.hostname = urlObj.hostname.replace(
'www.figma.com',
'embed.figma.com'
);

const normalizedUrl = urlObj.toString();

return (
<iframe
title={ title }
src={ normalizedUrl }
className="wp-storybook-figma-embed"
{ ...props }
/>
);
}

export default FigmaEmbed;
4 changes: 4 additions & 0 deletions storybook/components/figma-embed/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.wp-storybook-figma-embed {
width: 100%;
border: none;
}
3 changes: 2 additions & 1 deletion storybook/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ const stories = [
process.env.NODE_ENV !== 'test' && './stories/**/*.story.@(js|tsx)',
process.env.NODE_ENV !== 'test' && './stories/**/*.mdx',
'../packages/block-editor/src/**/stories/*.story.@(js|tsx|mdx)',
'../packages/components/src/**/stories/*.story.@(js|tsx|mdx)',
'../packages/components/src/**/stories/*.story.@(js|tsx)',
'../packages/components/src/**/stories/*.mdx',
'../packages/icons/src/**/stories/*.story.@(js|tsx|mdx)',
'../packages/edit-site/src/**/stories/*.story.@(js|tsx|mdx)',
'../packages/dataviews/src/**/stories/*.story.@(js|tsx|mdx)',
Expand Down
Loading