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.

Sign up for GitHub

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

Components: Remove unexpected has-text class when empty children are passed to Button #44198

Merged
merged 9 commits into from
Sep 21, 2022
Merged

Components: Remove unexpected has-text class when empty children are passed to Button #44198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5c80024
Remove unexpected has-text class when empty children are passed to `B…
t-hamano Sep 15, 2022
c7bd31a
Update tests
t-hamano Sep 15, 2022
7ede583
Remove unnecessary change
t-hamano Sep 15, 2022
eaa2dd1
Add a test for the Tooltip case
t-hamano Sep 16, 2022
39737e1
Merge branch 'trunk' into fix/components-button-has-text-class
t-hamano Sep 16, 2022
12fe7db
Fix typo
t-hamano Sep 16, 2022
6d75c4a
Update PostSavedState unit test snapshot
t-hamano Sep 16, 2022
a9e383d
Update chagelog
t-hamano Sep 17, 2022
02ee824
Add button focus to 'should render a button element with has-text whe…
t-hamano Sep 20, 2022
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 packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Bug Fix

- `Button`: Remove unexpected `has-text` class when empty children are passed ([#44198](https://github.com/WordPress/gutenberg/pull/44198)).
- The `LinkedButton` to unlink sides in `BoxControl`, `BorderBoxControl` and `BorderRadiusControl` have changed from a rectangular primary button to an icon-only button, with a sentence case tooltip, and default-size icon for better legibility. The `Button` component has been fixed so when `isSmall` and `icon` props are set, and no text is present, the button shape is square rather than rectangular.

### New Features
Expand Down
8 changes: 7 additions & 1 deletion packages/components/src/button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,12 @@ export function Button( props, ref ) {
'components-button__description'
);

const hasChildren =
children?.[ 0 ] &&
children[ 0 ] !== null &&
// Tooltip should not considered as a child
children?.[ 0 ]?.props?.className !== 'components-tooltip';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Tooltip is not excluded as a child, it will be considered to have children when moused over and has-text class will be given.
I couldn't find a good approach to not regard Tooltip as a child.
Is there a better way?

Copy link
Member

Choose a reason for hiding this comment

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

Oof, I can't think of a better way either. I think it's good enough for a temporary measure though.

Long term, I'm hoping it could be resolved by the Tooltip rewrite. Maybe that will remove the need to inject direct children?

Otherwise, we could perhaps add an official API to Button that allows consumers to set some kind of "ignore me as a visible child" flag in CSS. So like if you add a designated CSS class, e.g. <Button><div className="ignore-as-components-button-child" /></Button>, Button could exclude it from its hasChildren check.

Another way might be to only apply .has-text when the child is actually a ReactText. I'm not sure, but I'm guessing that there are not a lot of use cases that involve a icon prop and an element child with text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, we could perhaps add an official API to Button that allows consumers to set some kind of "ignore me as a visible child" flag in CSS. So like if you add a designated CSS class, e.g.

, Button could exclude it from its hasChildren check.

Perhaps I think this new API should be considered in the overall Button component in #44042 for consideration.

Another way might be to only apply .has-text when the child is actually a ReactText. I'm not sure, but I'm guessing that there are not a lot of use cases that involve a icon prop and an element child with text.

Is it correct that a use case with a icon prop and an element child with text is something like the following?

<Button icon={ help } variant="primary">
	Push Me
</Button>

I've seen such usage many times in my experience, so I think that the has-text class should be added.

Long term, I'm hoping it could be resolved by the Tooltip rewrite. Maybe that will remove the need to inject direct children?

Yes, that's the challenge I must address in #42753 😅

Copy link
Member

Choose a reason for hiding this comment

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

Another way might be to only apply .has-text when the child is actually a ReactText. I'm not sure, but I'm guessing that there are not a lot of use cases that involve a icon prop and an element child with text.

Is it correct that a use case with a icon prop and an element child with text is something like the following?

Sorry, this was unclear. By "icon prop and an element child with text" I meant something like:

// *Element* child with text
<Button icon={ help }>
	<span>Push Me</span>
</Button>

in contrast to a text child:

// *Text* child
<Button icon={ help }>
	Push Me
</Button>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I understand.
From what I have found on GitHub Code Search, it appears that "an element child with text" is almost never used.
For now, however, I think it would have less impact to consider both "element child with text" and "text child" as "having text".

This specification may need to be reconsidered in relation to #44042, but I think it is reasonable in this PR.
If it is not a problem, I would like to merge it 🚀

Copy link
Member

Choose a reason for hiding this comment

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

Yep, sounds good! 🚢


const classes = classnames( 'components-button', className, {
'is-secondary': variant === 'secondary',
'is-primary': variant === 'primary',
Expand All @@ -101,7 +107,7 @@ export function Button( props, ref ) {
'is-busy': isBusy,
'is-link': variant === 'link',
'is-destructive': isDestructive,
'has-text': !! icon && !! children,
'has-text': !! icon && hasChildren,
'has-icon': !! icon,
} );

Expand Down
36 changes: 36 additions & 0 deletions packages/components/src/button/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { plusCircle } from '@wordpress/icons';
* Internal dependencies
*/
import Button from '../';
import { Tooltip } from '../../';

jest.mock( '../../icon', () => () => <div data-testid="test-icon" /> );

Expand Down Expand Up @@ -75,6 +76,41 @@ describe( 'Button', () => {
expect( screen.getByRole( 'button' ) ).toHaveClass( 'is-pressed' );
} );

it( 'should render a button element with has-text when children are passed', async () => {
render( <Button icon={ plusCircle }>Children</Button> );
await screen.getByRole( 'button' ).focus();
expect( screen.getByRole( 'button' ) ).toHaveClass( 'has-text' );
} );

it( 'should render a button element without has-text when children are not passed', async () => {
render( <Button icon={ plusCircle }></Button> );
expect( screen.getByRole( 'button' ) ).not.toHaveClass(
'has-text'
);
} );

it( 'should render a button element without has-text when children are empty fragment', async () => {
render(
<Button icon={ plusCircle }>
<></>
</Button>
);
expect( screen.getByRole( 'button' ) ).not.toHaveClass(
'has-text'
Copy link
Member

Choose a reason for hiding this comment

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

Eventually I'm hoping we can move these kinds of tests to visual regression testing because they are more robust than checking for class names ✨

);
} );

it( 'should render a button element without has-text when a button wrapped in Tooltip', async () => {
render(
<Tooltip text="Help text">
<Button icon={ plusCircle } />
</Tooltip>
);
expect( screen.getByRole( 'button' ) ).not.toHaveClass(
'has-text'
);
} );

it( 'should add a disabled prop to the button', () => {
render( <Button disabled /> );

Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/post-saved-state/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ exports[`PostSavedState returns a disabled button if the post is not saveable 1`
<button
aria-disabled="true"
aria-label="Save draft"
class="components-button has-text has-icon"
class="components-button has-icon"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to the failure of this unit test I have found another problem.

When the screen width is narrow, only an icon is shown on the Button that indicates the save status. The has-text class should not be given, but in the trunk, The has-text class is shown for a certain status, which causes strange behaviour.

PostSavedState_trunk.mp4

This PR will solve this problem as well. Therefore, this snapshot update is intended.

PostSavedState_pr.mp4

type="button"
>
<svg
Expand Down