Skip to content

Commit f4f6553

Browse files
committed
Cleaned up from rebase and feedback
1 parent 36f273f commit f4f6553

File tree

7 files changed

+131
-56
lines changed

7 files changed

+131
-56
lines changed

packages/react-core/src/components/Button/Button.tsx

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -154,20 +154,14 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
154154
countOptions,
155155
...props
156156
}: ButtonProps) => {
157-
if (isFavorite && !ariaLabel && !props['aria-labelledby']) {
158-
// eslint-disable-next-line no-console
159-
console.error(
160-
'Button: Each favorite button must have a unique accessible name provided via aria-label or aria-labelledby'
161-
);
162-
}
163157
if (isHamburger && ![true, false].includes(isExpanded)) {
164158
// eslint-disable-next-line no-console
165159
console.error(
166160
'Button: when the isHamburger property is passed in, you must also pass in a boolean value to the isExpanded property. It is expected that a hamburger button controls the expansion of other content.'
167161
);
168162
}
169-
// TODO: Remove isSettings || isHamburger conditional in breaking change to throw this warning for any button that does not have children or aria name
170-
if ((isSettings || isHamburger) && !ariaLabel && !children && !props['aria-labelledby']) {
163+
// TODO: Remove isSettings || isHamburger || isFavorite conditional in breaking change to throw this warning for any button that does not have children or aria name
164+
if ((isSettings || isHamburger || isFavorite) && !ariaLabel && !children && !props['aria-labelledby']) {
171165
// eslint-disable-next-line no-console
172166
console.error(
173167
'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
@@ -179,7 +173,6 @@ const ButtonBase: React.FunctionComponent<ButtonProps> = ({
179173
const isButtonElement = Component === 'button';
180174
const isInlineSpan = isInline && Component === 'span';
181175
const isIconAlignedAtEnd = iconPosition === 'end' || iconPosition === 'right';
182-
const shouldForcePlainVariant = isSettings || isHamburger;
183176
const shouldOverrideIcon = isSettings || isHamburger || isFavorite;
184177

185178
const preventedEvents = inoperableEvents.reduce(

packages/react-core/src/components/Button/__tests__/Button.test.tsx

Lines changed: 74 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -410,7 +410,7 @@ describe('Settings button', () => {
410410
test('Throws console error when isSettings is true and neither children, aria-label nor aria-lablledby are passed', () => {
411411
const consoleError = jest.spyOn(console, 'error').mockImplementation();
412412

413-
render(<Button isSettings isExpanded />);
413+
render(<Button isSettings />);
414414

415415
expect(consoleError).toHaveBeenCalledWith(
416416
'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
@@ -474,32 +474,83 @@ describe('Settings button', () => {
474474
});
475475
});
476476

477-
test(`Renders basic button`, () => {
478-
const { asFragment } = render(<Button aria-label="basic button">Basic Button</Button>);
479-
expect(asFragment()).toMatchSnapshot();
480-
});
477+
describe('Favorite button', () => {
478+
// TODO: Remove isFavorite in breaking change to throw error for any button that does not have children or aria name
479+
test('Throws console error when isFavorite is true and neither children, aria-label nor aria-lablledby are passed', () => {
480+
const consoleError = jest.spyOn(console, 'error').mockImplementation();
481481

482-
test(`Renders with class ${styles.modifiers.favorite} when isFavorite is true`, () => {
483-
render(<Button isFavorite />);
484-
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.favorite);
485-
});
482+
render(<Button isFavorite />);
486483

487-
test(`Renders with class ${styles.modifiers.favorited} when isFavorite is true and isFavorited is true`, () => {
488-
render(<Button isFavorite isFavorited />);
489-
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.favorited);
490-
});
484+
expect(consoleError).toHaveBeenCalledWith(
485+
'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
486+
);
487+
});
491488

492-
test(`Does not render with class ${styles.modifiers.favorite} when isFavorite is false`, () => {
493-
render(<Button />);
494-
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.favorite);
495-
});
489+
// TODO: Remove isFavorite in breaking change to throw error for any button that does not have children or aria name
490+
test('Does not throw console error when isFavorite is true and children is passed', () => {
491+
const consoleError = jest.spyOn(console, 'error').mockImplementation();
492+
493+
render(<Button isFavorite>Settings</Button>);
494+
495+
expect(consoleError).not.toHaveBeenCalledWith(
496+
'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
497+
);
498+
});
499+
500+
// TODO: Remove isFavorite in breaking change to throw error for any button that does not have children or aria name
501+
test('Does not throw console error when isFavorite is true and aria-label is passed', () => {
502+
const consoleError = jest.spyOn(console, 'error').mockImplementation();
503+
504+
render(<Button isFavorite aria-label="Test" />);
505+
506+
expect(consoleError).not.toHaveBeenCalledWith(
507+
'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
508+
);
509+
});
510+
511+
// TODO: Remove isFavorite in breaking change to throw error for any button that does not have children or aria name
512+
test('Does not throw console error when isFavorite is true and aria-labelledby is passed', () => {
513+
const consoleError = jest.spyOn(console, 'error').mockImplementation();
514+
515+
render(
516+
<>
517+
<div id="label">Test</div>
518+
<Button isFavorite aria-labelledby="label" />
519+
</>
520+
);
521+
522+
expect(consoleError).not.toHaveBeenCalledWith(
523+
'Button: you must provide either visible text content or an accessible name via the aria-label or aria-labelledby properties.'
524+
);
525+
});
526+
527+
test(`Renders with class ${styles.modifiers.favorite} when isFavorite is true`, () => {
528+
render(<Button isFavorite />);
529+
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.favorite);
530+
});
531+
532+
test(`Renders with class ${styles.modifiers.favorited} when isFavorite is true and isFavorited is true`, () => {
533+
render(<Button isFavorite isFavorited />);
534+
expect(screen.getByRole('button')).toHaveClass(styles.modifiers.favorited);
535+
});
536+
537+
test(`Does not render with class ${styles.modifiers.favorite} when isFavorite is false`, () => {
538+
render(<Button />);
539+
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.favorite);
540+
});
496541

497-
test(`Does not render with class ${styles.modifiers.favorited} when isFavorite is true and isFavorited is false`, () => {
498-
render(<Button isFavorite />);
499-
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.favorited);
542+
test(`Does not render with class ${styles.modifiers.favorited} when isFavorite is true and isFavorited is false`, () => {
543+
render(<Button isFavorite />);
544+
expect(screen.getByRole('button')).not.toHaveClass(styles.modifiers.favorited);
545+
});
546+
547+
test('Overrides icon prop when isFavorite is true', () => {
548+
render(<Button isFavorite icon={<div>Icon content</div>} />);
549+
expect(screen.queryByText('Icon content')).not.toBeInTheDocument();
550+
});
500551
});
501552

502-
test('Overrides icon prop when isFavorite is true', () => {
503-
render(<Button isFavorite icon={<div>Icon content</div>} />);
504-
expect(screen.queryByText('Icon content')).not.toBeInTheDocument();
553+
test(`Renders basic button`, () => {
554+
const { asFragment } = render(<Button aria-label="basic button">Basic Button</Button>);
555+
expect(asFragment()).toMatchSnapshot();
505556
});

packages/react-core/src/components/Button/__tests__/__snapshots__/Button.test.tsx.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ exports[`Renders basic button 1`] = `
55
<button
66
aria-label="basic button"
77
class="pf-v6-c-button pf-m-primary"
8-
data-ouia-component-id="OUIA-Generated-Button-primary-57"
8+
data-ouia-component-id="OUIA-Generated-Button-primary-66"
99
data-ouia-component-type="PF6/Button"
1010
data-ouia-safe="true"
1111
type="button"

packages/react-core/src/components/Button/examples/Button.md

Lines changed: 40 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,74 +23,83 @@ import QuestionCircleIcon from '@patternfly/react-icons/dist/esm/icons/question-
2323
PatternFly supports several button styling variants to be used in different scenarios as needed. The following button variants can be assigned using the `variant` property.
2424

2525
```ts file="./ButtonVariations.tsx"
26+
2627
```
2728

28-
| Variant | Description|
29-
| --- | ---|
30-
| Primary | Primary buttons are the most visually prominent variant. Use for the most important call to action.|
31-
| Secondary | Secondary buttons are less visually prominant than primary buttons. Use for general actions on a page that require less emphasis than primary buttons. |
32-
| Tertiary | Tertiary buttons are the least visually prominent variant. Use to retain a classic button format with less emphasis than primary or secondary buttons. |
33-
| Danger | Danger buttons may be used for actions that are potentially destructive or difficult/impossible to undo. Danger is an independent variant, but all button variants may use the the `isDanger` property to apply similar styling. |
34-
| Warning | Warning buttons may be used for actions that change an important setting or behavior, but not in a destructive or irreversible way. |
35-
| Link | Links are labeled, but have no background or border. Use for actions that require less emphasis, actions that navigate users to another page within the same window, and/or actions that navigate to external pages in a new window. Links may be placed inline with text using the `isInline` property.|
36-
| Plain | Plain buttons have no styling and are intended to be labeled with icons. |
37-
| Control | Control buttons can be labeled with text or icons. Primarily intended to be paired with other controls in an [input group](/components/input-group). |
38-
| Stateful | Stateful buttons are ideal for displaying the state of notifications. They have 3 states: `read`, `unread` and `attention`.
29+
| Variant | Description |
30+
| --------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
31+
| Primary | Primary buttons are the most visually prominent variant. Use for the most important call to action. |
32+
| Secondary | Secondary buttons are less visually prominant than primary buttons. Use for general actions on a page that require less emphasis than primary buttons. |
33+
| Tertiary | Tertiary buttons are the least visually prominent variant. Use to retain a classic button format with less emphasis than primary or secondary buttons. |
34+
| Danger | Danger buttons may be used for actions that are potentially destructive or difficult/impossible to undo. Danger is an independent variant, but all button variants may use the the `isDanger` property to apply similar styling. |
35+
| Warning | Warning buttons may be used for actions that change an important setting or behavior, but not in a destructive or irreversible way. |
36+
| Link | Links are labeled, but have no background or border. Use for actions that require less emphasis, actions that navigate users to another page within the same window, and/or actions that navigate to external pages in a new window. Links may be placed inline with text using the `isInline` property. |
37+
| Plain | Plain buttons have no styling and are intended to be labeled with icons. |
38+
| Control | Control buttons can be labeled with text or icons. Primarily intended to be paired with other controls in an [input group](/components/input-group). |
39+
| Stateful | Stateful buttons are ideal for displaying the state of notifications. They have 3 states: `read`, `unread` and `attention`. |
3940

4041
### Disabled buttons
4142

4243
To indicate that an action is currently unavailable, all button variations can be disabled using the `isDisabled` property.
4344

4445
```ts file="./ButtonDisabled.tsx"
46+
4547
```
4648

4749
### Small buttons
4850

4951
To fit into tight spaces, primary, secondary, tertiary, danger, and warning button variations can be made smaller using the `"sm"` value for the `size` property.
5052

5153
```ts file="./ButtonSmall.tsx"
54+
5255
```
5356

5457
### Call to action (CTA) buttons
5558

56-
CTA buttons and links direct users to complete an action. Primary, secondary, tertiary, and link button variants can be styled as CTAs using the `"lg"` value for the `size` property.
59+
CTA buttons and links direct users to complete an action. Primary, secondary, tertiary, and link button variants can be styled as CTAs using the `"lg"` value for the `size` property.
5760

5861
```ts file="./ButtonCallToAction.tsx"
62+
5963
```
6064

6165
### Block level buttons
6266

6367
Block level buttons span the full width of the parent element and can be enabled using the `isBlock` property.
6468

6569
```ts file="./ButtonBlock.tsx"
70+
6671
```
6772

6873
### Progress indicators
6974

70-
Progress indicators can be added to buttons to identify that an action is in progress after a click.
75+
Progress indicators can be added to buttons to identify that an action is in progress after a click.
7176

7277
```ts file="./ButtonProgress.tsx"
78+
7379
```
7480

7581
### Links as buttons
7682

7783
Buttons that link to another resource may take the form of primary, secondary, tertiary, or link variants. Use `component="a"` and an `href` property to designate the button's target link.
7884

7985
```ts file="./ButtonLinks.tsx"
86+
8087
```
8188

8289
### Inline link as span
8390

8491
Inline links should use `component="span"` and the `isInline` property to wrap inline with surrounding text.
8592

8693
```ts file="./ButtonInlineSpanLink.tsx"
94+
8795
```
8896

8997
### Custom component
9098

9199
In addition to being able to pass a string to the `component` property, you can provide more fine-tuned customization by passing a callback that returns a component.
92100

93101
```ts file="./ButtonCustomComponent.tsx"
102+
94103
```
95104

96105
### Aria-disabled examples
@@ -102,44 +111,61 @@ Buttons that are aria-disabled are similar to normal disabled buttons, except th
102111
Unlike normal disabled buttons, aria-disabled buttons can support tooltips. Furthermore, aria-disabled buttons can operate as links, which also support tooltips.
103112

104113
```ts file="./ButtonAriaDisabled.tsx"
114+
105115
```
106116

107117
### Button with count
108118

109119
Buttons can display a `count` in the form of a badge to indicate some value or number by passing in the `countOptions` prop as a `BadgeCountObject` object. The `BadgeCountObject` object will handle `count`, `isRead`, and `className` props for the badge count.
110120

111121
```ts file="./ButtonWithCount.tsx"
122+
112123
```
113124

114125
### Plain with no padding
115126

116127
To display a plain/icon button inline with text, use `noPadding` prop in addition to `variant="plain"`.
117128

118129
```ts file="./ButtonPlainHasNoPadding.tsx"
130+
119131
```
120132

121133
### Stateful
122134

123135
Stateful buttons are ideal for displaying the state of notifications. Use `variant="stateful"` alongside with the `state` property, which can have these 3 values: `read`, `unread` (used as default) and `attention` (which means unread and requires attention).
124136

125137
```ts file="./ButtonStateful.tsx"
138+
126139
```
127140

141+
## Animated examples
142+
143+
The following `<Button>` implementations have animations built into them. When using one of the following implementations, the `icon` property will be overridden due to the animations needing specific icons internally.
144+
128145
### Favorite
129146

130147
You can pass both the `isFavorite` and `variant="plain"` properties into the `<Button>` to create a favorite button. Passing the `isFavorited` property will determine the current favorited state and update styling accordingly.
131148

132149
```ts file = "./ButtonFavorite.tsx"
150+
133151
```
134152

135-
### Settings
153+
### Settings
154+
155+
For a "settings" button with animations on hover and focus, you can pass the `isSettings` property into either a `variant="plain"` or `variant="control"` button.
136156

137157
```ts file="./ButtonSettings.tsx"
158+
138159
```
139160

140161
### Hamburger
141162

163+
For a "hamburger" button (`fa-bars`) that will animate based on the `hamburgerVariant` property, you can pass the `isHamburger` property. This will render the default icon for a hamburger button, and updating the `hamburgerVariant` property with a value of either "expand" or "collapse" will change the icon accordingly with an animation on hover and focus.
164+
165+
When used within a PatternFly `<Masthead>`, the animations and icon updates will occur automatically. See one of our [full screen page demos](/components/page/react-demos) to see this in action.
166+
142167
```ts file="./ButtonHamburger.tsx"
168+
143169
```
144170

145171
## Using router links
Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
1+
import styles from '@patternfly/react-styles/css/components/Button/button';
2+
import { css } from '@patternfly/react-styles';
3+
14
// Because this is such a specific icon that requires being wrapped in a pf-v[current version]-c-button element,
25
// we don't want to export this to consumers nor include it in the react-icons package as a custom icon.
36
export const hamburgerIcon = (
47
<svg viewBox="0 0 10 10" className="pf-v6-c-button--hamburger-icon pf-v6-svg" width="1em" height="1em">
5-
<path className="pf-v6-c-button--hamburger-icon--top" d="M1,1 L9,1"></path>
6-
<path className="pf-v6-c-button--hamburger-icon--middle" d="M1,5 L9,5"></path>
7-
<path className="pf-v6-c-button--hamburger-icon--arrow" d="M1,5 L1,5 L1,5"></path>
8-
<path className="pf-v6-c-button--hamburger-icon--bottom" d="M9,9 L1,9"></path>
8+
<path className={css(styles.buttonHamburgerIconTop)} d="M1,1 L9,1"></path>
9+
<path className={css(styles.buttonHamburgerIconMiddle)} d="M1,5 L9,5"></path>
10+
<path className={css(styles.buttonHamburgerIconArrow)} d="M1,5 L1,5 L1,5"></path>
11+
<path className={css(styles.buttonHamburgerIconBottom)} d="M9,9 L1,9"></path>
912
</svg>
1013
);

packages/react-core/src/components/MenuToggle/examples/MenuToggle.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,8 @@ import { MenuToggle, Badge } from '@patternfly/react-core';
7272
7373
### Settings toggle
7474
75+
To create a "settings" menu toggle that will animate on hover and focus, you can pass the `isSettings` property in. Doing so will override the `icon` property, as a specific icon is used internally for the animations.
76+
7577
```ts file="./MenuToggleSettings.tsx"
7678

7779
```
Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
1-
import { MenuToggle, Flex } from '@patternfly/react-core';
1+
import { Fragment } from 'react/jsx-runtime';
2+
import { MenuToggle } from '@patternfly/react-core';
23

34
export const MenuToggleSettings: React.FunctionComponent = () => (
4-
<Flex>
5-
<MenuToggle isSettings>Settings</MenuToggle>
6-
<MenuToggle isSettings variant="plain" aria-label="Settings" />
7-
</Flex>
5+
<Fragment>
6+
<MenuToggle isSettings>Settings</MenuToggle> <MenuToggle isSettings variant="plain" aria-label="Settings" />
7+
</Fragment>
88
);

0 commit comments

Comments
 (0)