feat: improve `disabled` prop handling in `Button` and `ButtonGroup`

cheton commented 2 months ago

PR Type

Enhancement, Documentation

Description

Added support for disabled prop in Button, ButtonGroup, Checkbox, and Radio components.
Utilized nullish coalescing operator for handling disabled and other props.
Updated documentation to reflect changes in disabled prop for ButtonGroup, CheckboxGroup, and RadioGroup.
Added build-public script to package.json.

Changes walkthrough 📝

Relevant files

Enhancement

4 files

Button.js Add support for `disabled` prop in `Button` packages/react/src/button/Button.js Added support for `disabled` prop in `Button`. Used nullish coalescing operator for `disabled` and `orientation`. Adjusted logic to respect `ButtonGroup` context.	+9/-6
ButtonGroup.js Add `disabled` prop to `ButtonGroup` packages/react/src/button/ButtonGroup.js Added `disabled` prop to `ButtonGroup`. Updated context to include `disabled`.	+7/-1
Checkbox.js Update `Checkbox` to use nullish coalescing packages/react/src/checkbox/Checkbox.js Used nullish coalescing operator for `disabled` and `name`. Adjusted logic for `CheckboxGroup` context.	+2/-2
Radio.js Update `Radio` to use nullish coalescing packages/react/src/radio/Radio.js Used nullish coalescing operator for `disabled` and `name`. Adjusted logic for `RadioGroup` context.	+2/-2

Documentation

4 files

tonic-ui-927.md `Add changeset for disabled button group support` .changeset/tonic-ui-927.md - Added changeset for supporting disabled button group.	+5/-0
index.page.mdx Document `disabled` prop for `ButtonGroup` packages/react-docs/pages/components/button-group/index.page.mdx - Documented `disabled` prop for `ButtonGroup`.	+1/-0
index.page.mdx Update `disabled` prop documentation for `CheckboxGroup` packages/react-docs/pages/components/checkbox-group/index.page.mdx - Updated default value of `disabled` prop for `CheckboxGroup`.	+1/-1
index.page.mdx Update `disabled` prop documentation for `RadioGroup` packages/react-docs/pages/components/radio-group/index.page.mdx - Updated default value of `disabled` prop for `RadioGroup`.	+1/-1

Configuration changes

1 files

package.json

Add `build-public` script to package.json

package.json - Added `build-public` script to package.json.

+1/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codesandbox[bot] commented 2 months ago

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders
Open Preview

changeset-bot[bot] commented 2 months ago

🦋 Changeset detected

Latest commit: 8c645885ddfcf66124203ee4ec87a91cd68e0763

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | --------------- | ----- | | @tonic-ui/react | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codiumai-pr-agent-free[bot] commented 2 months ago

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/trendmicro-frontend/tonic-ui/commit/8c645885ddfcf66124203ee4ec87a91cd68e0763)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

Possible Bug
The `disabled` prop is being set using nullish coalescing, which might not work as expected if `disabled` is explicitly set to `false`. Possible Bug
The `disabled` prop is being set using nullish coalescing, which might not work as expected if `disabled` is explicitly set to `false`. Code Smell
The `disabled` prop is being set using nullish coalescing, which might not work as expected if `disabled` is explicitly set to `false`.

codesandbox-ci[bot] commented 2 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.

Project coverage is 77.80%. Comparing base (da62c8c) to head (8c64588). Report is 1 commits behind head on v2.

Files with missing lines	Patch %	Lines
packages/react/src/button/Button.js	33.33%	2 Missing :warning:
packages/react/src/button/ButtonGroup.js	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## v2 #930 +/- ## ========================================== + Coverage 77.26% 77.80% +0.53% ========================================== Files 403 403 Lines 6620 6632 +12 ========================================== + Hits 5115 5160 +45 + Misses 1505 1472 -33 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

trendmicro-frontend-bot commented 2 months ago

Tonic UI Demo

On 2024-09-24 07:37:56 +0000, PR #930 (8c645885ddfcf66124203ee4ec87a91cd68e0763) was successfully deployed. You can view it at the following link: https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-930/

cheton commented 2 months ago

/describe

codiumai-pr-agent-free[bot] commented 2 months ago

PR Description updated to latest commit (https://github.com/trendmicro-frontend/tonic-ui/commit/5162afc4d6a6e4610c0b510fc6f8fb72fb845d8f)

[ ] Copy walkthrough table to "Files Changed" Tab

cheton commented 2 months ago

/review

cheton commented 2 months ago

/improve

codiumai-pr-agent-free[bot] commented 2 months ago

Persistent review updated to latest commit https://github.com/trendmicro-frontend/tonic-ui/commit/d2d4a6859456e4304c020177c265171c24b0a16e

cheton commented 2 months ago

/improve

codiumai-pr-agent-free[bot] commented 2 months ago

PR Code Suggestions ✨

Latest suggestions up to 8c64588

Category	Suggestion	Score
Enhancement	Move the name conflict check outside the conditional block for consistent execution ___ Consider moving the name conflict check and warning outside of the `if` `(checkboxGroupContext)` block to ensure it's always performed, regardless of whether a checkbox group context exists. [packages/react/src/checkbox/Checkbox.js [60-67]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-d5ae55a366f7b950201c3eed96f52580aac6a13b4d1ff587e33bc597a67a5c51R60-R67) ```diff const isNameConflict = (!isNullish(name) && !isNullish(checkboxGroupName) && (name !== checkboxGroupName)); -if (process.env.NODE_ENV !== 'production' && isNameConflict && !isNameConflict.current) { +if (process.env.NODE_ENV !== 'production' && isNameConflict && !isNameConflictRef.current) { // Log the warning message only once console.error( `Warning: The \`Checkbox\` has a \`name\` prop ("${name}") that conflicts with the \`CheckboxGroup\`'s \`name\` prop ("${checkboxGroupName}")` ); isNameConflictRef.current = true; } +if (checkboxGroupContext) { + // ... existing code ... +} + ``` Suggestion importance[1-10]: 8 Why: Moving the name conflict check outside the conditional block ensures that the check is always performed, which is a significant improvement for consistency and reliability in the code.	8
Maintainability	Add a comment explaining the purpose of the getMemoizedState function ___ Consider adding a comment to explain the purpose of the `getMemoizedState` function and how it's used in the context of the `ButtonGroup` component. [packages/react/src/button/ButtonGroup.js [25-30]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-7eb5a03e77d6ee9639b58e062a8f452133274079a68e68b495303d15d3f96393R25-R30) ```diff +// Create a memoized context object to prevent unnecessary re-renders +// when the ButtonGroup props haven't changed const context = getMemoizedState({ disabled, orientation, size, variant, }); ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 5 Why: Adding a comment improves code readability and maintainability by clarifying the purpose of the `getMemoizedState` function, though it is not crucial for functionality.	5

Previous suggestions

Suggestions up to commit d2d4a68

Category	Suggestion	Score
Enhancement	Simplify null checks using modern JavaScript features ___ Replace the `isNullish` function calls with the more concise and commonly used nullish coalescing operator (??) for checking if `name` and `radioGroupName` are not null or undefined. [packages/react/src/radio/Radio.js [58]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-902e8001f5f749a8725aa09ca99a7020fb3923fca8861a8478b575090d353573R58-R58) ```diff -const isNameConflict = (!isNullish(name) && !isNullish(radioGroupName) && (name !== radioGroupName)); +const isNameConflict = (name != null && radioGroupName != null && name !== radioGroupName); ``` Suggestion importance[1-10]: 8 Why: The suggestion correctly simplifies the null checks using modern JavaScript syntax, improving code readability and maintainability.	8
Performance	Move console.error to useEffect for better performance ___ Consider moving the console.error statement outside of the component's render cycle to improve performance. You could use a custom hook or a useEffect with appropriate dependencies to log the warning only when necessary. [packages/react/src/checkbox/Checkbox.js [60-64]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-d5ae55a366f7b950201c3eed96f52580aac6a13b4d1ff587e33bc597a67a5c51R60-R64) ```diff -if (process.env.NODE_ENV !== 'production' && isNameConflict) { - console.error( - `Warning: The \`Checkbox\` has a \`name\` prop ("${name}") that conflicts with the \`CheckboxGroup\`'s \`name\` prop ("${checkboxGroupName}")` - ); -} +useEffect(() => { + if (process.env.NODE_ENV !== 'production' && isNameConflict) { + console.error( + `Warning: The \`Checkbox\` has a \`name\` prop ("${name}") that conflicts with the \`CheckboxGroup\`'s \`name\` prop ("${checkboxGroupName}")` + ); + } +}, [isNameConflict, name, checkboxGroupName]); ``` Suggestion importance[1-10]: 7 Why: The suggestion improves performance by moving the console.error statement to a useEffect hook, ensuring it only runs when necessary, which is a good practice for optimizing component rendering.	7

✅ Suggestions up to commit 6aa564d

Category	Suggestion	Score
Enhancement	✅ Use nullish coalescing operator for more precise boolean handling ___ Suggestion Impact: The suggestion to use the nullish coalescing operator (??) instead of the logical OR operator (\|\|) for the variable 'disabled' was implemented. code diff: ```diff - disabled = buttonGroupDisabled \|\| disabled; + disabled = buttonGroupDisabled ?? disabled; ``` ___ Consider using the nullish coalescing operator (??) instead of the logical OR operator (\|\|) when checking for `buttonGroupDisabled`. This ensures that `false` values are not treated as falsy. [packages/react/src/button/Button.js [34]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-37592a959e1ed17e0fa9b850e25ae31a73268d7cddb4c47cb5567d1d87307b3bR34-R34) ```diff -disabled = buttonGroupDisabled \|\| disabled; +disabled = buttonGroupDisabled ?? disabled; ``` Suggestion importance[1-10]: 7 Why:	7
Enhancement	Use object property shorthand for more concise code ___ Consider using object property shorthand for the `context` object creation to make the code more concise and easier to read. [packages/react/src/button/ButtonGroup.js [25-30]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-7eb5a03e77d6ee9639b58e062a8f452133274079a68e68b495303d15d3f96393R25-R30) ```diff -const context = getMemoizedState({ - disabled, - orientation, - size, - variant, -}); +const context = getMemoizedState({ disabled, orientation, size, variant }); ``` Suggestion importance[1-10]: 7 Why:	7
Best practice	Simplify object destructuring by removing unnecessary spread operator ___ Consider destructuring `buttonGroupDisabled` and `buttonGroupOrientation` directly in the object destructuring statement to improve code readability and reduce the number of lines. [packages/react/src/button/Button.js [28-33]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-37592a959e1ed17e0fa9b850e25ae31a73268d7cddb4c47cb5567d1d87307b3bR28-R33) ```diff const { disabled: buttonGroupDisabled, orientation: buttonGroupOrientation, size: buttonGroupSize, variant: buttonGroupVariant, -} = { ...buttonGroupContext }; +} = buttonGroupContext; ``` Suggestion importance[1-10]: 7 Why:	7
Maintainability	Add a description comment for the new build script ___ Consider adding a description for the new `build-public` script to improve maintainability and help other developers understand its purpose. [package.json [10]](https://github.com/trendmicro-frontend/tonic-ui/pull/930/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R10-R10) ```diff -"build-public": "lerna exec --no-private -- yarn build", +"build-public": "lerna exec --no-private -- yarn build # Builds only public packages", ``` Suggestion importance[1-10]: 7 Why:	7

cheton commented 2 months ago

/review

codiumai-pr-agent-free[bot] commented 2 months ago

Persistent review updated to latest commit https://github.com/trendmicro-frontend/tonic-ui/commit/8c645885ddfcf66124203ee4ec87a91cd68e0763

trendmicro-frontend / tonic-ui