fix: misc Button implementations not using isIconButton prop #37382
Conversation
WalkthroughThe changes in this pull request involve updates to several components across the application, primarily focusing on the addition of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
hetunandu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/packages/design-system/ads/src/Documentation/components/CodeBlock.tsx (1)
Line range hint
11-21: Consider enhancing accessibility for the copy functionality.The component could benefit from improved accessibility features.
Consider these improvements:
function CodeBlock({ code }: CodeBlockProps) { return ( <Wrapper onClick={() => { code && navigator.clipboard.writeText(code.toString()); toast.show("Copied to clipboard", { kind: "success" }); }} + role="region" + aria-label="Code block with copy functionality" > <Code>{code}</Code> - <Button isIconButton kind="tertiary" startIcon="copy-control" /> + <Button + isIconButton + kind="tertiary" + startIcon="copy-control" + aria-label="Copy code to clipboard" + /> </Wrapper> ); }app/client/src/pages/AppViewer/Navigation/TopStacked.tsx (1)
Line range hint
115-126: Consider DRYing up the ScrollBtnContainer props.The scroll button configurations are duplicated. Consider extracting common props to reduce duplication.
+ const getScrollButtonProps = (isLeft: boolean) => ({ + isIconButton: true, + kind: "tertiary" as const, + size: "sm" as const, + startIcon: isLeft ? "left-arrow-2" : "right-arrow-2", + onMouseDown: () => startScrolling(isLeft), + onMouseLeave: stopScrolling, + onMouseUp: stopScrolling, + onTouchEnd: stopScrolling, + onTouchStart: () => startScrolling(isLeft), + }); {tabsScrollable && ( <ScrollBtnContainer className="left-0 scroll-arrows" - isIconButton - kind="tertiary" - onMouseDown={() => startScrolling(true)} - onMouseLeave={stopScrolling} - onMouseUp={stopScrolling} - onTouchEnd={stopScrolling} - onTouchStart={() => startScrolling(true)} - size="sm" - startIcon="left-arrow-2" + {...getScrollButtonProps(true)} visible={shouldShowLeftArrow} /> )}Also applies to: 157-168
app/client/src/components/editorComponents/CodeEditor/index.tsx (1)
Line range hint
1713-1725: Consider simplifying button implementationThe button implementation could be improved for better maintainability:
- The visibility logic could be simplified by combining the className conditions
- The onClick handler could be extracted to a named method
Consider this refactor:
-className={classNames( - "commands-button invisible", - !showSlashCommandButton && "!hidden", -)} +className={classNames("commands-button invisible", { + "!hidden": !showSlashCommandButton +})} -onClick={() => { - const newValue = - typeof this.props.input.value === "string" - ? this.props.input.value + "/" - : "/"; - this.updatePropertyValue(newValue); -}} +onClick={this.handleSlashCommand}Add this method to the class:
private handleSlashCommand = () => { const newValue = typeof this.props.input.value === "string" ? this.props.input.value + "/" : "/"; this.updatePropertyValue(newValue); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/client/packages/design-system/ads/src/Documentation/components/CodeBlock.tsx(1 hunks)app/client/src/components/editorComponents/CodeEditor/index.tsx(1 hunks)app/client/src/pages/AppViewer/Navigation/TopStacked.tsx(2 hunks)app/client/src/pages/Editor/CustomWidgetBuilder/Preview/Debugger/helpDropdown.tsx(1 hunks)
🔇 Additional comments (4)
app/client/packages/design-system/ads/src/Documentation/components/CodeBlock.tsx (1)
19-19: LGTM! Button prop addition aligns with sizing fix objectives.
The addition of isIconButton prop correctly addresses the button sizing issue while maintaining the existing copy functionality.
app/client/src/pages/Editor/CustomWidgetBuilder/Preview/Debugger/helpDropdown.tsx (1)
23-28: LGTM! Button changes align with size fix requirements.
The addition of isIconButton prop along with tertiary kind and small size maintains the icon-only appearance while fixing the minimum width issues.
app/client/src/pages/AppViewer/Navigation/TopStacked.tsx (1)
115-115: LGTM! Changes align with button size standardization.
The addition of isIconButton prop to scroll buttons is consistent with the PR's objective of fixing button size issues.
Also applies to: 157-157
app/client/src/components/editorComponents/CodeEditor/index.tsx (1)
1713-1713: Confirm the isIconButton prop addition aligns with design system
The addition of isIconButton prop to the Button component aligns with the PR's objective of fixing button size issues.
Let's verify consistent usage across other icon buttons:
✅ Verification successful
The isIconButton prop is consistently used across the codebase for icon-only buttons
The isIconButton prop is correctly used in this context as it's a well-established pattern in the codebase:
- It's defined in the Button component's interface (
ButtonProps) in the design system - Used consistently across 80+ icon-only button instances in the codebase
- Properly handled in the Button component's styles to adjust padding and width for icon buttons
- Follows the design system's implementation for icon-only buttons
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other icon button usages to ensure consistency
rg -U 'isIconButton.{0,50}Button' --type ts --type tsx
Length of output: 84
Script:
#!/bin/bash
# Search for other icon button usages with corrected file type
rg -U 'isIconButton' --type typescript --type tsx
# Also search for Button component imports to understand the component's origin
rg -U 'import.*Button' --type typescript
# Search for Button component definition to understand prop interface
ast-grep --pattern 'interface ButtonProps {
$$$
}'
Length of output: 2762
Script:
#!/bin/bash
# Try with explicit file extension search
rg -U 'isIconButton' -g '*.ts' -g '*.tsx'
# Look for Button component usage patterns
rg -U '<Button.*?>' -g '*.tsx' -A 2
# Search for icon-related button patterns
rg -U 'Button.*icon' -g '*.tsx' -A 2
Length of output: 72526
…horg#37382) ## Description Fixes issues introduced when we added min width for buttons in appsmithorg#37367 ## Automation /ok-to-test tags="@tag.Sanity" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/11831136814> > Commit: dce8dcd > Workflow: `PR Automation test suite` > Tags: `@tag.Sanity` > Spec: `` > <hr>Thu, 14 Nov 2024 05:12:11 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No
Description
Fixes issues introduced when we added min width for buttons in #37367
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11831136814
Commit: dce8dcd
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 14 Nov 2024 05:53:10 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements
Bug Fixes