feat(Button): consume Penta tokens and update examples by adamviktora · Pull Request #9934 · patternfly/patternfly-react

adamviktora · 2024-01-04T15:43:50Z

What: Closes #9907

This is a followup to a core PR: patternfly/patternfly#6127

Breaking change: renamed isActive prop to isClicked, codemod issue followup: Button - Penta updates pf-codemods#546
introduced: hasNoPadding prop, stateful variant + state prop
updated examples to use Flex so buttons are not that tight next to each other

patternfly-build · 2024-01-04T15:53:15Z

Preview: https://patternfly-react-pr-9934.surge.sh

A11y report: https://patternfly-react-pr-9934-a11y.surge.sh

nicolethoen · 2024-01-04T16:04:58Z

can we open a code mod issue to track this breaking change?

adamviktora · 2024-01-04T16:12:10Z

sure! codemod followup: patternfly/pf-codemods#546
we could keep the property name as isActive I think so we don't have that many codemods, but to be consistent with core, isClicked or clicked will be better

jeff-phillips-18 · 2024-01-05T15:24:50Z

we could keep the property name as isActive I think so we don't have that many codemods, but to be consistent with core, isClicked or clicked will be better

How is isClicked better? What is the modifier supposed to convey? I wouldn't think I would ever have to tell a button programmatically that it is clicked. isActive makes more sense to me if the intent is to show that the button is currently selected.

mcoker · 2024-01-05T18:57:33Z

hey @jeff-phillips-18! With v6/penta, we introduced tokens (global vars) for colors and borders and stuff that we're currently calling "clicked" (you can see here, just look for --clicked) as a sort of catch-all for the variation of a token when something is selected, active, expanded/open, etc. This state in the button isn't terribly prescriptive, and could be used for any of those cases.

One issue with calling it "active" is we have modifiers classes for certain things to apply the hover/focus/active styling when the thing isn't actually hovered/focused/active. Eg, in a typeahead select, the text input in the toggle has actual focus, so when you're using the keyboard to navigate the open menu items, we apply .pf-m-focus to force the :focus styling on those items to mimic tabbing through menu items with actual browser focus. We're trying to reserve the active/focus/etc naming for when that prop/class represents that particular state, and are calling something "clicked" to represent the catch-all states.

Does that make sense or do you think we should reconsider anything? Naming things is hard, so your feedback is much appreciated!

mcoker · 2024-01-05T19:00:21Z

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

+  /** Sets state of the stateful button variant. Default is "unread" */
+  state?: 'read' | 'unread' | 'attention';
+  /** Applies no padding on a plain button variant. Use when plain button is placed inline with text */
+  noPadding?: boolean;


mcoker

LGTM! 👍👍

andrew-ronaldson

Approved! Good to have you back @adamviktora !

jeff-phillips-18 · 2024-01-11T12:31:39Z

Does that make sense or do you think we should reconsider anything? Naming things is hard, so your feedback is much appreciated!

I get it now. Naming is hard, clicked is as good as anything. Thanks for the explanation @mcoker

tlabaj

Looks good! Just s few small comments

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

tlabaj · 2024-01-12T15:15:39Z

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

-xtest('Renders with class pf-m-active when isActive = true', () => {
-  render(<Button isActive>Active Button</Button>);
-  expect(screen.getByRole('button')).toHaveClass('pf-m-active');
+xtest('Renders with class pf-m-clicked when isClicked = true', () => {


Can we reenable this one?

Well, there will no longer be the isActive prop and the pf-m-active modifier in V6, it is renamed to isClicked. So I don't think we should reenable that test.

It looks like you modified it for the isClicked. Does the test fail? I that why it is still disabled?

I have opened this issue #9989

Oh, I did not realize that when it is marked as xtest and not test that it means the test is disabled.

fix PR: #10013

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

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

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

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

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

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

lboehling

lgtm!

adamviktora · 2024-01-16T09:39:24Z

@mcoker I am noticing a slight difference in the border color of a stateful read button variant when applying the pf-m-clicked modifier vs actually clicking the button and thus applying :focus or :hover pseudo class.

Applying pf-m-clicked class:

--pf-v5-c-button--BorderColor: var(--pf-v5-c-button--m-clicked--BorderColor)

Focus or hover:

--pf-v5-c-button--BorderColor: var(--pf-v5-c-button--hover--BorderColor)

mcoker · 2024-01-16T20:14:39Z

Hey @adamviktora, here is the design - it looks like that's intentional?

tlabaj

LGTM.

patternfly-build · 2024-01-16T22:04:09Z

Your changes have been released in:

@patternfly/react-charts@8.0.0-alpha.7
@patternfly/react-code-editor@6.0.0-alpha.11
@patternfly/react-core@6.0.0-alpha.11
@patternfly/react-docs@7.0.0-alpha.11
@patternfly/react-icons@6.0.0-alpha.7
@patternfly/react-integration@6.0.0-alpha.8
demo-app-ts@5.1.1-alpha.10
@patternfly/react-styles@6.0.0-alpha.7
@patternfly/react-table@6.0.0-alpha.11
@patternfly/react-tokens@6.0.0-alpha.7

Thanks for your contribution! 🎉

feat(Button): consume Penta tokens and update examples

4c3dd28

adamviktora requested review from a team, mcoker and tlabaj and removed request for a team January 4, 2024 15:43

wise-king-sullyman linked an issue Jan 4, 2024 that may be closed by this pull request

(Button): Consume Penta tokens #9907

Closed

adamviktora added Breaking change 💥 this change requires a major release and has API changes. Penta labels Jan 4, 2024

adamviktora mentioned this pull request Jan 4, 2024

Button - Penta updates patternfly/pf-codemods#546

Closed

mcoker reviewed Jan 5, 2024

View reviewed changes

mcoker approved these changes Jan 5, 2024

View reviewed changes

tlabaj added the ux review label Jan 5, 2024

tlabaj requested review from andrew-ronaldson and lboehling January 5, 2024 20:22

andrew-ronaldson approved these changes Jan 5, 2024

View reviewed changes

tlabaj added the css review label Jan 9, 2024

fix (Button docs): add aria-label to pass a11y tests

27fd544

nicolethoen approved these changes Jan 11, 2024

View reviewed changes

tlabaj requested changes Jan 12, 2024

View reviewed changes

lboehling approved these changes Jan 12, 2024

View reviewed changes

adamviktora added 2 commits January 16, 2024 10:26

fix(Button): rename noPadding to hasNoPadding

d076af7

docs(Button): update fragments

8572a61

adamviktora requested a review from tlabaj January 16, 2024 10:22

tlabaj approved these changes Jan 16, 2024

View reviewed changes

tlabaj merged commit d7a3b29 into patternfly:v6 Jan 16, 2024

tlabaj mentioned this pull request Jan 16, 2024

Reenable Button test #9989

Closed

adamviktora mentioned this pull request Jan 17, 2024

(Button): Consume Penta tokens #9907

Closed

Conversation

adamviktora commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolethoen commented Jan 4, 2024

Uh oh!

adamviktora commented Jan 4, 2024

Uh oh!

jeff-phillips-18 commented Jan 5, 2024

Uh oh!

mcoker commented Jan 5, 2024

Uh oh!

mcoker Jan 5, 2024

Choose a reason for hiding this comment

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-ronaldson left a comment

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jan 11, 2024

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tlabaj Jan 12, 2024

Choose a reason for hiding this comment

Uh oh!

adamviktora Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

tlabaj Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

tlabaj Jan 16, 2024

Choose a reason for hiding this comment

Uh oh!

adamviktora Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

adamviktora Jan 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lboehling left a comment

Choose a reason for hiding this comment

Uh oh!

adamviktora commented Jan 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcoker commented Jan 16, 2024

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Jan 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

adamviktora commented Jan 4, 2024 •

edited

Loading

patternfly-build commented Jan 4, 2024 •

edited

Loading

adamviktora commented Jan 16, 2024 •

edited

Loading