feat: Add copy icon to text and table cell info-panel segments#3382
feat: Add copy icon to text and table cell info-panel segments#3382dblythy wants to merge 1 commit into
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
Bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughAdds a reusable CopyButton component to AggregationPanelComponents and threads a new showNote prop through TextElement, KeyValueElement, and TableElement to enable copy-to-clipboard with notifications for text and table segments. AggregationPanel.js passes showNote accordingly, and SCSS hover selectors are updated for the new elements. ChangesCopy icon extension for text and table segments
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: mtrezza 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/AggregationPanel/AggregationPanelComponents.js (2)
22-28: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo guard against empty/undefined
textfor the copy icon.Unlike
TableElement, which only rendersCopyButtonwhen the value is notundefined/null/'',TextElementalways renders it. Iftextis empty/undefined, clicking still copies"undefined"/empty string and shows the confirmation note, which is misleading.🩹 Proposed fix
export const TextElement = ({ text, style, showNote }) => ( <div className={`text-element ${styles.textElement}`} style={style}> <p>{text}</p> - <CopyButton value={text} showNote={showNote} /> + {text !== undefined && text !== null && text !== '' && ( + <CopyButton value={text} showNote={showNote} /> + )} </div> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AggregationPanel/AggregationPanelComponents.js` around lines 22 - 28, The TextElement component always renders CopyButton even when text is empty or undefined, unlike TableElement’s guarded behavior. Update TextElement in AggregationPanelComponents to conditionally render CopyButton only when text is not null, undefined, or an empty string, so the copy action and showNote only appear for valid values.
6-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCopyButton is not keyboard-accessible.
The clickable
<span>only handlesonClick; there's norole="button",tabIndex,onKeyDown, oraria-label. Keyboard-only or screen-reader users cannot trigger the copy action.♿ Proposed fix
return ( - <span className={styles.copyIcon} onClick={handleCopy}> + <span + className={styles.copyIcon} + onClick={handleCopy} + role="button" + tabIndex={0} + aria-label="Copy to clipboard" + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + handleCopy(); + } + }} + > <Icon name="clone-icon" width={12} height={12} fill="currentColor" /> </span> );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AggregationPanel/AggregationPanelComponents.js` around lines 6 - 20, The CopyButton component is only clickable with a mouse, so update the CopyButton span to be keyboard- and screen-reader accessible. In AggregationPanelComponents.js, add button semantics to the CopyButton element by using an appropriate role, tabIndex, aria-label, and an onKeyDown handler that triggers handleCopy for keyboard activation, while keeping the existing copy logic and showNote behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/AggregationPanel/AggregationPanelComponents.js`:
- Around line 22-28: The TextElement component always renders CopyButton even
when text is empty or undefined, unlike TableElement’s guarded behavior. Update
TextElement in AggregationPanelComponents to conditionally render CopyButton
only when text is not null, undefined, or an empty string, so the copy action
and showNote only appear for valid values.
- Around line 6-20: The CopyButton component is only clickable with a mouse, so
update the CopyButton span to be keyboard- and screen-reader accessible. In
AggregationPanelComponents.js, add button semantics to the CopyButton element by
using an appropriate role, tabIndex, aria-label, and an onKeyDown handler that
triggers handleCopy for keyboard activation, while keeping the existing copy
logic and showNote behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9b6f654-c6e3-43bb-aa45-144e05b77a8b
📒 Files selected for processing (3)
src/components/AggregationPanel/AggregationPanel.jssrc/components/AggregationPanel/AggregationPanel.scsssrc/components/AggregationPanel/AggregationPanelComponents.js
1 participant
Closes #2898
Extends the hover copy-to-clipboard icon (previously only on key-value info-panel segments, added in #2871) to text segments and individual table cells.
Text item
Before
After
Table cell
Before
After
Summary by CodeRabbit
New Features
Bug Fixes