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

Enhancement : Badge Component #66555

Merged
merged 24 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from 14 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
1 change: 1 addition & 0 deletions docs/tool/manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const componentPaths = glob( 'packages/components/src/*/**/README.md', {
'packages/components/src/menu/README.md',
'packages/components/src/tabs/README.md',
'packages/components/src/custom-select-control-v2/README.md',
'packages/components/src/badge/README.md',
],
} );
const packagePaths = glob( 'packages/*/package.json' )
Expand Down
37 changes: 37 additions & 0 deletions packages/components/src/badge/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Badge
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved

<!-- This file is generated automatically and cannot be edited directly. Make edits via TypeScript types and TSDocs. -->

<p class="callout callout-info">See the <a href="https://wordpress.github.io/gutenberg/?path=/docs/components-badge--docs">WordPress Storybook</a> for more detailed, interactive documentation.</p>

## Props

### `as`

Component type that will be used to render the badge component.

- Type: `ElementType`
- Required: No
- Default: `'div'`

### `className`

Additional classes for the badge component.

- Type: `string`
- Required: No

### `context`

Badge variant.

- Type: `"neutral" | "info" | "success" | "warning" | "error"`
- Required: No
- Default: `neutral`

### `children`

Element to display inside the badge.

- Type: `ReactNode`
- Required: Yes
5 changes: 5 additions & 0 deletions packages/components/src/badge/docs-manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"$schema": "../../schemas/docs-manifest.json",
"displayName": "Badge",
"filePath": "./index.tsx"
}
67 changes: 67 additions & 0 deletions packages/components/src/badge/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/**
* External dependencies
*/
import clsx from 'clsx';

/**
* WordPress dependencies
*/
import { info, caution, error, published } from '@wordpress/icons';

/**
* Internal dependencies
*/
import type { BadgeProps } from './types';
import Icon from '../icon';

function Badge( {
className,
as: Component = 'div',
context = 'neutral',
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
children,
...props
}: BadgeProps ) {
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
/**
* Returns an icon based on the badge context.
*
* @return {JSX.Element | null} The corresponding icon for the provided context.
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
*/
function contextBasedIcon(): JSX.Element | null {
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
switch ( context ) {
case 'info':
return info;
case 'warning':
return caution;
case 'error':
return error;
case 'success':
return published;
default:
return null;
}
}

return (
<Component
className={ clsx(
'components-badge',
`components-badge--${ context }`,
context !== 'neutral' && 'components-badge--has-icon',
className
) }
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
aria-label={ `${ context }-badge` }
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
{ ...props }
>
{ context !== 'neutral' && (
<Icon
icon={ contextBasedIcon() }
size={ 20 }
Copy link
Contributor

Choose a reason for hiding this comment

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

There aren't many (any?) examples of icons rendering at 20px in the software, but I don't mind it here. What do you think @jasmussen?

Screenshot 2024-12-02 at 13 28 30

24px looks a bit odd next to the smaller text:

Screenshot 2024-12-02 at 13 29 12

Choose a reason for hiding this comment

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

+1 on the 20px rendering! That works well for the visual consistency.
Thank you @jameskoster for covering it and your work on these icons!

BTW In the initial designs I even had it set to 16px.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, thanks for the ping, sorry for the belated response.

I have semi strong opinions on this, all related to the icons being carefully designed to be displayed at 24x24 precisely, so that the pixels render correctly. Even if they are SVGs meant to scale, the pixels won't alias perfectly at any resolution.

That's a long winded way of saying: if we are going to scale the icons down, we should do it according to some careful and deliberate math, so the pixels alias as well as possible. And we should do it very rarely: it should be an exception to the rule to use scaled icons, essentially, just like how we have heuristics for typography sizes. This badge can be such an exception, certainly.

So about that math: the icons are 24x24 with a 1.5px stroke width. At 20x20, the stroke width would be 1.25px. That would render blurry both at 1x screens, and at 2x screens (2.50px). If we scaled the icon to 16 (33% smaller icon), the stroke width would be exactly 1px, so it would be crisp at both 1x and 2x screens.

How would 16x16 look in that context, can you try? If the math checks out, it should look very crisp, even if smaller. And smaller in this case would be good insofar as it looks intentional: it's smaller enough to be significant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the context, we should document this, and maybe even restrict the size prop on Icon to specific values.

16px seems a bit small, but I'll take it over 24px:

Screenshot 2024-12-04 at 13 51 39

Copy link
Contributor

@jasmussen jasmussen Dec 5, 2024

Choose a reason for hiding this comment

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

I quite like that, because it makes the icon feel like it's part of the text, almost like an in-text glyph rather than an icon. But curious what others think.

Choose a reason for hiding this comment

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

@jameskoster @jasmussen I completely agree with the concerns raised here. The 16px icon rendered differently in Figma for me, but I agree with @jameskoster that it looks a bit off in the screenshot above. At the same time, I agree with @jasmussen that size-wise, it blends nicely with the text, which was also my goal.

The issue, though, is that the icon wasn’t designed for this size—its stroke width looks a tiny bit too thin when rescaled, and it doesn’t fill the crop area effectively.

Would it make sense to add a set of icons to the library specifically for badges at this smaller size? These could have adjusted stroke widths and proportions to better suit their context.

BTW Google does it well :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to add a set of icons to the library specifically for badges at this smaller size? These could have adjusted stroke widths and proportions to better suit their context.

This is a bit of a large undertaking, and even now we are missing a build process (#38850) so this is likely not feasible in the foreseeable future. In that vein, I'd either keep the icons at 16px size, try 18px, just use 24x24, or omit the icon entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

I vote we use the current icon at 16px. The resulting strokes are exactly 1px which I agree is a little thin, but should render sharply.

In the future it might be beneficial to redraw the icons to use stroke-width, then we can be a bit more precise/flexible. I discuss this a bit in #65786.

Copy link
Contributor

Choose a reason for hiding this comment

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

The resulting strokes are exactly 1px which I agree is a little thin, but should render sharply.

Agreed. This is especially important for 1x resolution monitors, which is still the overwhelming majority of desktop monitors.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the future it might be beneficial to redraw the icons to use stroke-width, then we can be a bit more precise/flexible. I discuss this a bit in #65786.

This would also potentially open the door for some animation fun. Agreed this is a big undertaking.

fill="currentColor"
/>
) }
{ children }
</Component>
);
}

export default Badge;
49 changes: 49 additions & 0 deletions packages/components/src/badge/stories/index.story.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
* External dependencies
*/
import type { Meta, StoryObj } from '@storybook/react';

/**
* Internal dependencies
*/
import Badge from '..';

const meta = {
component: Badge,
title: 'Components/Containers/Badge',
argTypes: {
className: {
control: { type: 'text' },
},
as: {
control: { type: 'select' },
options: [ 'div', 'span' ],
},
context: {
control: { type: 'select' },
options: [ 'neutral', 'info', 'warning', 'error', 'success' ],
},
children: {
control: { type: null },
},
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
},
tags: [ 'status-private' ],
} satisfies Meta< typeof Badge >;

export default meta;

type Story = StoryObj< typeof meta >;

export const Default: Story = {
args: {
children: 'Code is Poetry',
context: 'neutral',
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
},
};

export const WithContext: Story = {
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
args: {
children: 'Code is Poetry',
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
context: 'success',
},
};
38 changes: 38 additions & 0 deletions packages/components/src/badge/styles.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
@mixin badge-color-variant( $base-color ) {
background-color: mix($white, $base-color, 90%);
color: mix($black, $base-color, 50%);
Copy link
Member

Choose a reason for hiding this comment

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

@WordPress/gutenberg-design Have we considered what we want to happen when a Badge is placed on a dark background? (This can be a follow-up)

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the concern?

Copy link
Member

Choose a reason for hiding this comment

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

Just thinking forward about theming. The default badge is subtle on a white background, but eye catching on a dark one. Color differences are also harder to perceive, I think, especially in isolation.

Default badge Success badge

FWIW, the default badge looks like this when rendered in the current theming system:

Default badge in dark theming

(To be clear, these issues aren't blockers for this PR to land.)

}

$badge-colors: (
"info": #3858e9,
"warning": $alert-yellow,
"error": $alert-red,
"success": $alert-green,
);

.components-badge {
background: $gray-100;
color: $gray-800;
padding: 0 $grid-unit-10;
min-height: $grid-unit-30;
border-radius: $radius-small;
font-size: $font-size-small;
font-weight: 400;
flex-shrink: 0;
line-height: $font-line-height-small;
width: fit-content;
display: flex;
align-items: center;
gap: 2px;

&--has-icon {
padding-left: $grid-unit-05;
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
}

// Generate color variants
@each $type, $color in $badge-colors {
&.components-badge--#{$type} {
@include badge-color-variant( $color );
}
}
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
}
66 changes: 66 additions & 0 deletions packages/components/src/badge/test/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/**
* External dependencies
*/
import { render, screen } from '@testing-library/react';

/**
* Internal dependencies
*/
import Badge from '..';

describe( 'Badge', () => {
it( 'should render correctly with default props', () => {
render( <Badge>Code is Poetry</Badge> );
const badge = screen.getByText( 'Code is Poetry' );
expect( badge ).toBeInTheDocument();
expect( badge.tagName ).toBe( 'DIV' ); // Default element should be a div
expect( badge ).toHaveClass( 'components-badge' );
} );

it( 'should render as a span when specified', () => {
render( <Badge as="span">Code is Poetry</Badge> );
const badge = screen.getByText( 'Code is Poetry' );
expect( badge.tagName ).toBe( 'SPAN' );
expect( badge ).toHaveClass( 'components-badge' );
} );

it( 'should render as a custom element when specified', () => {
render( <Badge as="article">Code is Poetry</Badge> );
const badge = screen.getByText( 'Code is Poetry' );
expect( badge.tagName ).toBe( 'ARTICLE' );
expect( badge ).toHaveClass( 'components-badge' );
} );

it( 'should combine custom className with default class', () => {
render( <Badge className="custom-class">Code is Poetry</Badge> );
const badge = screen.getByText( 'Code is Poetry' );
expect( badge ).toHaveClass( 'components-badge' );
expect( badge ).toHaveClass( 'custom-class' );
} );

it( 'should render children correctly', () => {
render(
<Badge>
<span>Nested</span> Content
</Badge>
);

const badge = screen.getByText( ( content, element ) => {
return element?.classList?.contains( 'components-badge' ) ?? false;
} );

expect( badge ).toBeInTheDocument();
expect( badge ).toHaveClass( 'components-badge' );
expect( badge ).toHaveTextContent( 'Nested Content' );

const nestedSpan = screen.getByText( 'Nested' );
expect( nestedSpan.tagName ).toBe( 'SPAN' );
} );

it( 'should pass through additional props', () => {
render( <Badge data-testid="custom-badge">Code is Poetry</Badge> );
const badge = screen.getByTestId( 'custom-badge' );
expect( badge ).toHaveTextContent( 'Code is Poetry' );
expect( badge ).toHaveClass( 'components-badge' );
} );
} );
27 changes: 27 additions & 0 deletions packages/components/src/badge/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* External dependencies
*/
import type { ElementType, ReactNode } from 'react';

export type BadgeProps = {
/**
* Additional classes for the badge component.
*/
className?: string;
/**
* Component type that will be used to render the badge component.
*
* @default 'div'
*/
as?: ElementType;
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
/**
* Badge variant.
*
* @default 'generic'
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
*/
context?: 'neutral' | 'info' | 'success' | 'warning' | 'error';
/**
* Element to display inside the badge.
*/
children: ReactNode;
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved
};
1 change: 1 addition & 0 deletions packages/components/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ export {
} from './higher-order/with-focus-return';
export { default as withNotices } from './higher-order/with-notices';
export { default as withSpokenMessages } from './higher-order/with-spoken-messages';
export { default as Badge } from './badge';
Vrishabhsk marked this conversation as resolved.
Show resolved Hide resolved

// Private APIs.
export { privateApis } from './private-apis';
2 changes: 2 additions & 0 deletions packages/components/src/private-apis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import Theme from './theme';
import { Tabs } from './tabs';
import { kebabCase } from './utils/strings';
import { lock } from './lock-unlock';
import Badge from './badge';

export const privateApis = {};
lock( privateApis, {
Expand All @@ -17,4 +18,5 @@ lock( privateApis, {
Theme,
Menu,
kebabCase,
Badge,
} );
1 change: 1 addition & 0 deletions packages/components/src/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
// Components
@import "./animate/style.scss";
@import "./autocomplete/style.scss";
@import "./badge/styles.scss";
@import "./button-group/style.scss";
@import "./button/style.scss";
@import "./checkbox-control/style.scss";
Expand Down
3 changes: 3 additions & 0 deletions packages/icons/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

- Add new `caution` icon.
- Add new `error` icon.

## 10.13.0 (2024-11-27)

## 10.12.0 (2024-11-16)
Expand Down
2 changes: 2 additions & 0 deletions packages/icons/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export { default as caption } from './library/caption';
export { default as capturePhoto } from './library/capture-photo';
export { default as captureVideo } from './library/capture-video';
export { default as category } from './library/category';
export { default as caution } from './library/caution';
export { default as chartBar } from './library/chart-bar';
export { default as check } from './library/check';
export { default as chevronDown } from './library/chevron-down';
Expand Down Expand Up @@ -84,6 +85,7 @@ export { default as download } from './library/download';
export { default as edit } from './library/edit';
export { default as envelope } from './library/envelope';
export { default as external } from './library/external';
export { default as error } from './library/error';
export { default as file } from './library/file';
export { default as filter } from './library/filter';
export { default as flipHorizontal } from './library/flip-horizontal';
Expand Down
16 changes: 16 additions & 0 deletions packages/icons/src/library/caution.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* WordPress dependencies
*/
import { SVG, Path } from '@wordpress/primitives';

const caution = (
Copy link
Member

Choose a reason for hiding this comment

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

@WordPress/gutenberg-design I just want to make sure we measure twice and cut once here because publishing a new icon is kind of irreversible 😄

Would it be worth renaming these so it follows the convention of foo + fooFilled like we do for the other pairs like this? For example, change the canonical name of warning to cautionFilled (maintaining back compat with an aliased export of course).

Caution and warning icons Help icon pair

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes, good suggestion. Just to clarify; that means any existing instances of warning would map to cautionFilled?

We should update the cautionFilled icon so that the details match the new caution icon too. Do you think it makes sense to do that in this PR, or separately?

Copy link
Member

Choose a reason for hiding this comment

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

that means any existing instances of warning would map to cautionFilled?

Correct 👍

Do you think it makes sense to do that in this PR, or separately?

Yes, let me pull the icon changes out of this PR so we can address it separately with proper care. I'll prepare a PR so you can push up changes to the SVG.

@Vrishabhsk Keeping this PR on hold for just a bit more. Thank you for your patience!

Copy link
Member

Choose a reason for hiding this comment

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

@jameskoster Updates to the SVG can be pushed here: #67895

<SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg">
<Path
fill-rule="evenodd"
clip-rule="evenodd"
d="M5.5 12a6.5 6.5 0 1 0 13 0 6.5 6.5 0 0 0-13 0ZM12 4a8 8 0 1 0 0 16 8 8 0 0 0 0-16Zm-.75 12v-1.5h1.5V16h-1.5Zm0-8v5h1.5V8h-1.5Z"
/>
</SVG>
);

export default caution;
16 changes: 16 additions & 0 deletions packages/icons/src/library/error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* WordPress dependencies
*/
import { SVG, Path } from '@wordpress/primitives';

const error = (
<SVG viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg">
<Path
fill-rule="evenodd"
clip-rule="evenodd"
d="M12.218 5.377a.25.25 0 0 0-.436 0l-7.29 12.96a.25.25 0 0 0 .218.373h14.58a.25.25 0 0 0 .218-.372l-7.29-12.96Zm-1.743-.735c.669-1.19 2.381-1.19 3.05 0l7.29 12.96a1.75 1.75 0 0 1-1.525 2.608H4.71a1.75 1.75 0 0 1-1.525-2.608l7.29-12.96ZM12.75 17.46h-1.5v-1.5h1.5v1.5Zm-1.5-3h1.5v-5h-1.5v5Z"
/>
</SVG>
);

export default error;
Loading
Loading