Just icon #2
Just icon #2
Conversation
This new property gives the option to just display the Icons in the buttons
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant InputClickEdit
participant Button
User->>InputClickEdit: Configure with iconsOnly=true
InputClickEdit->>Button: Render only icons
Button-->>User: Display compact icon buttons
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
playground/src/App.tsx (2)
25-26: Replace<br />with proper CSS spacing.Using
<br />for spacing is not recommended in React. Consider using CSS margin or padding instead.- <br /> <InputClickEdit onInputChange={handleChange} value={value} justIcons />Add a CSS class to create spacing:
.input-spacing { margin-top: 1rem; }
26-26: Consider adding visual indicators to demonstrate the difference.Since both instances share the same value and handler, it might not be immediately clear to users what makes them different. Consider:
- Using different initial values
- Adding labels or comments to highlight the difference between regular and icon-only modes
src/InputClickEdit/InputClickEdit.tsx (2)
24-24: Consider grouping related props in the interface.The
justIconsprop is related toshowIcons, but they're separated in the interface. Consider grouping them together and adding JSDoc comments to clarify their relationship.iconPosition?: "left" | "right"; + /** Show icons in buttons */ showIcons?: boolean; + /** Show only icons without button labels */ justIcons?: boolean;
94-95: Consider extracting button content into a separate component.The button content rendering logic is duplicated between save and edit buttons. Consider extracting it into a reusable component.
type ButtonContentProps = { icon: React.ReactNode; label: React.ReactNode; showIcons: boolean; justIcons: boolean; }; const ButtonContent = ({ icon, label, showIcons, justIcons }: ButtonContentProps) => ( <> {(showIcons || justIcons) && icon} {!justIcons && label} </> );Then use it in both buttons:
<button className={cn(buttonBaseClassName, saveButtonClassName)} onClick={handleSave} > - {displayIcons && saveIcon} - {!justIcons && saveButtonLabel} + <ButtonContent + icon={saveIcon} + label={saveButtonLabel} + showIcons={showIcons} + justIcons={justIcons} + /> </button>Also applies to: 105-106
README.md (2)
52-52: Clarify the relationship betweenshowIconsandjustIcons.The description of
justIconscould be more explicit about its relationship withshowIcons. Consider updating it to:-| justIcons | boolean | false | Show only icons without button labels | +| justIcons | boolean | false | When true, shows only icons without button labels. Note: This implicitly enables icons even if showIcons is false |
87-94: Enhance the icons-only example.The current example could be more comprehensive. Consider:
- Adding comments to explain the behavior
- Showing comparison with regular mode
- Demonstrating real-world use cases
### Icons Only (No Text Labels) + +// Icons-only mode is perfect for compact UIs or mobile views ```tsx +// Compact mode with just icons <InputClickEdit value="Icons only buttons" justIcons editIcon={<span>✎</span>} saveIcon={<span>✓</span>} /> + +// Compare with regular mode +<InputClickEdit + value="Regular mode" + showIcons + editIcon={<span>✎</span>} + saveIcon={<span>✓</span>} +/></blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between fec6b332f0fe4db571144c20b863060d151a6122 and 776580f103ed3c47d4e61f25672a9905c9629d9a. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `README.md` (2 hunks) * `playground/src/App.tsx` (1 hunks) * `src/InputClickEdit/InputClickEdit.tsx` (5 hunks) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| [styles.button]: true, | ||
| [styles.buttonReverse]: iconPosition === "right", | ||
| }; | ||
| const displayIcons = showIcons || justIcons; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify the icon display logic.
The displayIcons variable combines two different concepts:
showIcons- whether to show icons at alljustIcons- whether to show only icons
This might lead to confusion. Consider removing this variable and making the logic more explicit.
- const displayIcons = showIcons || justIcons;
return (
// ...
<button
className={cn(buttonBaseClassName, saveButtonClassName)}
onClick={handleSave}
>
- {displayIcons && saveIcon}
- {!justIcons && saveButtonLabel}
+ {(showIcons || justIcons) && saveIcon}
+ {!justIcons && saveButtonLabel}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const displayIcons = showIcons || justIcons; | |
| return ( | |
| // ... | |
| <button | |
| className={cn(buttonBaseClassName, saveButtonClassName)} | |
| onClick={handleSave} | |
| > | |
| {(showIcons || justIcons) && saveIcon} | |
| {!justIcons && saveButtonLabel} |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)playground/src/App.tsx(1 hunks)src/InputClickEdit/InputClickEdit.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- playground/src/App.tsx
🔇 Additional comments (3)
src/InputClickEdit/InputClickEdit.tsx (3)
24-24: LGTM! Type definition is well-structured.The new
iconsOnlyprop is correctly typed as an optional boolean and positioned logically near other icon-related props.
45-45: LGTM! Default prop value ensures backward compatibility.The default value of
falseforiconsOnlymaintains backward compatibility with existing usage.
76-76: Simplify the icon display logic.The
displayIconsvariable combines two different concepts:
showIcons- whether to show icons at alliconsOnly- whether to show only iconsThis might lead to confusion. Consider making the logic more explicit by using the conditions directly in the JSX.
- const displayIcons = showIcons || iconsOnly; return ( // ... <button className={cn(buttonBaseClassName, saveButtonClassName)} onClick={handleSave} > - {displayIcons && saveIcon} - {!iconsOnly && saveButtonLabel} + {(showIcons || iconsOnly) && saveIcon} + {!iconsOnly && saveButtonLabel}
| {displayIcons && saveIcon} | ||
| {!iconsOnly && saveButtonLabel} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA labels for icon-only buttons.
When iconsOnly is true, the buttons will lack text labels which could impact accessibility. Consider adding appropriate ARIA labels to maintain accessibility.
<button
className={cn(buttonBaseClassName, saveButtonClassName)}
onClick={handleSave}
+ aria-label={iconsOnly ? saveButtonLabel?.toString() : undefined}
>
{displayIcons && saveIcon}
{!iconsOnly && saveButtonLabel}
</button>
// ... and similarly for the edit button ...
<button
className={cn(buttonBaseClassName, editButtonClassName)}
onClick={onEditClick}
+ aria-label={iconsOnly ? editButtonLabel?.toString() : undefined}
>
{displayIcons && editIcon}
{!iconsOnly && editButtonLabel}
</button>Also applies to: 105-106
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/InputClickEdit/InputClickEdit.tsx (1)
94-95: Improve icon and label rendering logic.The current implementation could be more maintainable by extracting the rendering logic into helper functions.
Consider this refactor:
+ const renderButtonContent = (icon: React.ReactNode, label: React.ReactNode) => { + const showIcon = showIcons || iconsOnly; + return ( + <> + {showIcon && icon} + {!iconsOnly && label} + </> + ); + }; // In the button JSX: - {(showIcons || iconsOnly) && saveIcon} - {!iconsOnly && saveButtonLabel} + {renderButtonContent(saveIcon, saveButtonLabel)} - {(showIcons || iconsOnly) && editIcon} - {!iconsOnly && editButtonLabel} + {renderButtonContent(editIcon, editButtonLabel)}This approach:
- Reduces code duplication
- Makes the rendering logic more maintainable
- Keeps the JSX cleaner
Also applies to: 106-107
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
playground/src/App.tsx(1 hunks)src/InputClickEdit/InputClickEdit.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- playground/src/App.tsx
🔇 Additional comments (2)
src/InputClickEdit/InputClickEdit.tsx (2)
24-24: LGTM! Well-structured type definition.The
iconsOnlyprop is appropriately typed as an optional boolean and well-placed among other UI-related props.
45-45: LGTM! Sensible default value.The default value of
falsemaintains backward compatibility and follows the component's existing behavior.
| <button | ||
| className={cn(buttonBaseClassName, saveButtonClassName)} | ||
| onClick={handleSave} | ||
| aria-label={iconsOnly ? saveButtonLabel?.toString() : undefined} |
There was a problem hiding this comment.
Fix potential aria-label type issues.
The current aria-label implementation has potential issues:
toString()on ReactNode might not be safe as ReactNode can be null/undefined- Explicitly setting undefined is unnecessary as omitting the prop achieves the same result
Apply this fix:
- aria-label={iconsOnly ? saveButtonLabel?.toString() : undefined}
+ aria-label={iconsOnly && typeof saveButtonLabel === 'string' ? saveButtonLabel : undefined}
- aria-label={iconsOnly ? editButtonLabel?.toString() : undefined}
+ aria-label={iconsOnly && typeof editButtonLabel === 'string' ? editButtonLabel : undefined}Also applies to: 104-104
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 5 5
Lines 118 123 +5
Branches 5 5
=====================================
- Misses 113 118 +5
Partials 5 5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This PR adds the feature of just display the icon on the buttons of the component.
Summary by CodeRabbit
Release Notes
New Features
iconsOnlyprop toInputClickEditcomponent, allowing optional icon-only button display.Documentation
iconsOnlyproperty and example usage.Examples
InputClickEditcomponent instance in App showcasing icon-only mode.