Refactor Button component. #10824
8000
Open
Refactor Button component. #10824
Conversation
This file contains hidden or 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
|
Build files for 93cfaef are ready:
|
|
Size Change: +2.97 kB (+0.14%) Total Size: 2.13 MB ℹ️ View Unchanged
|
nfmohit
requested changes
Jun 5, 2025
| ? title || customizedTooltip || ariaLabel | ||
| : null; | ||
|
|
||
| const buttonComponent = ( |
There was a problem hiding this comment.
Since this variable is not reused anymore, we should get rid of it and render the component inline. What do you think?
| */ | ||
| import Tooltip from '../Tooltip'; | ||
|
|
||
| const MaybeTooltip = ( { |
There was a problem hiding this comment.
Our convention is to define components using the function keyword. Let's do that here.
Example:
export default function MaybeTooltip| */ | ||
| import Tooltip from '../Tooltip'; | ||
|
|
||
| const MaybeTooltip = ( { |
There was a problem hiding this comment.
Let's define prop types for this component.
| */ | ||
| import { forwardRef } from '@wordpress/element'; | ||
|
|
||
| const SemanticButton = forwardRef( |
There was a problem hiding this comment.
Let's define prop types for this component, or somehow reuse from the parent component. While we're at it, it would be nice to update any outdated prop types.
Comment on lines
19
to
35
| /** | ||
| * MaybeTooltip tests. | ||
| * | ||
| * Site Kit by Google, Copyright 2021 Google LLC | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * https://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ |
There was a problem hiding this comment.
Looks like this was duplicated.
Suggested change
| /** | |
| * MaybeTooltip tests. | |
| * | |
| * Site Kit by Google, Copyright 2021 Google LLC | |
| * | |
| * Licensed under the Apache License, Version 2.0 (the "License"); | |
| * you may not use this file except in compliance with the License. | |
| * You may obtain a copy of the License at | |
| * | |
| * https://www.apache.org/licenses/LICENSE-2.0 | |
| * | |
| * Unless required by applicable law or agreed to in writing, software | |
| * distributed under the License is distributed on an "AS IS" BASIS, | |
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| * See the License for the specific language governing permissions and | |
| * limitations under the License. | |
| */ |
There was a problem hiding this comment.
Solid tests here, thank you!
A very nitpicky observation: We seem to be capitalising "Tooltip" in the test case names, which I think we shouldn't. Please let me know what you think, thanks!
| /** | ||
| * SemanticButton tests. | ||
| * | ||
| * Site Kit by Google, Copyright 2021 Google LLC |
There was a problem hiding this comment.
Suggested change
| * Site Kit by Google, Copyright 2021 Google LLC | |
| * Site Kit by Google, Copyright 2025 Google LLC |
Comment on lines
57
to
88
| it( 'should apply danger class when danger prop is true', () => { | ||
| const { container } = render( | ||
| <SemanticButton danger>Test</SemanticButton> | ||
| ); | ||
| expect( container.firstChild ).toHaveClass( 'mdc-button--danger' ); | ||
| } ); | ||
|
|
||
| it( 'should apply inverse class when 8000 inverse prop is true', () => { | ||
| const { container } = render( | ||
| <SemanticButton inverse>Test</SemanticButton> | ||
| ); | ||
| expect( container.firstChild ).toHaveClass( 'mdc-button--inverse' ); | ||
| } ); | ||
|
|
||
| it( 'should apply tertiary class when tertiary prop is true', () => { | ||
| const { container } = render( | ||
| <SemanticButton tertiary>Test</SemanticButton> | ||
| ); | ||
| expect( container.firstChild ).toHaveClass( 'mdc-button--tertiary' ); | ||
| } ); | ||
|
|
||
| it( 'should apply callout classes when callout props are set', () => { | ||
| const { container } = render( | ||
| <SemanticButton callout calloutStyle="warning"> | ||
| Test | ||
| </SemanticButton> | ||
| ); | ||
| expect( container.firstChild ).toHaveClass( 'mdc-button--callout' ); | ||
| expect( container.firstChild ).toHaveClass( | ||
| 'mdc-button--callout-warning' | ||
| ); | ||
| } ); |
There was a problem hiding this comment.
Not very important, but this could be a nice spot to use it.each.
|
Thank you @nfmohit for reviewing the PR and adding your valuable suggestions. I've addressed the feedback and sending it your way to review those. Thanks again! |
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.
Summary
Addresses issue:
assets/js/googlesitekit/components-gm2/Button.js#10357Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist