Components: Remove unexpected has-text class when empty children are passed to Button
#44198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
commented
Sep 15, 2022
| children?.[ 0 ] && | ||
| children[ 0 ] !== null && | ||
| // Tooltip should not considered as a child | ||
| children?.[ 0 ]?.props?.className !== 'components-tooltip'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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>There was a problem hiding this comment.
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 🚀
t-hamano
commented
Sep 15, 2022
|
Size Change: +616 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
|
Thank you for the PR and test! 🙏 |
mirka
reviewed
Sep 15, 2022
| children?.[ 0 ] && | ||
| children[ 0 ] !== null && | ||
| // Tooltip should not considered as a child | ||
| children?.[ 0 ]?.props?.className !== 'components-tooltip'; |
There was a problem hiding this comment.
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.
| </Button> | ||
| ); | ||
| expect( screen.getByRole( 'button' ) ).not.toHaveClass( | ||
| 'has-text' |
There was a problem hiding this comment.
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 ✨
t-hamano
commented
Sep 16, 2022
| @@ -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" | |||
There was a problem hiding this comment.
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
mirka
approved these changes
Sep 19, 2022
…n children are passed' test
michalczaplinski
added
the
[Type] Bug
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix the problem found in #43802's comment.
What?
This PR fixes unexpected
has-textclass being given to aButtoncomponent when an empty child is passed to it.Why?
As I have investigated in this comment, when a
Buttoncomponent is placed directly under aTooltipcomponent, the Button component has an array with null as a child and is given thehas-textclass, which is not expected.Similarly, I found that not specifying a child, such as
<Button></Button>, or passing an empty fragment caused the same problem.I think that this problem is caused by the laxity of the conditions for checking whether the
Buttonhas children or not.How?
The conditions for granting the has-text class have been made stricter.
Testing Instructions
edit.jsin any of the blocks:Code
Screenshot and Screencast