-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-8326 - copy to clipboard button for warning bubble #16304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Complete implementation, but the images need to be updated
Removed extraneous dir from .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8326
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a “copy to clipboard” button to the InfoBubble’s error, warning, and info sections, implements a corresponding command in the view model, and wires up tooltip text and image resources.
- Introduce
CopyToClipboardCommand
inInfoBubbleViewModel
withCopyTextToClipboard
andCanCopyToClipboard
logic - Add copy buttons with hover/normal images and tooltip in three XAML sections
- Register new tooltip string in
.resx
files and include image assets in the project file
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/DynamoCoreWpf/Views/Preview/InfoBubbleView.xaml | Added three copy-to-clipboard buttons in the error, warning, and info sections |
src/DynamoCoreWpf/ViewModels/Preview/InfoBubbleViewModel.cs | Defined CopyToClipboardCommand , copy logic, and can-execute check |
src/DynamoCoreWpf/Properties/Resources.resx | Added CopyToClipboardTooltip string |
src/DynamoCoreWpf/Properties/Resources.en-US.resx | Added locale entry for CopyToClipboardTooltip |
src/DynamoCoreWpf/DynamoCoreWpf.csproj | Included copy_normal.png and copy_hover.png as resources |
Files not reviewed (1)
- src/DynamoCoreWpf/Properties/Resources.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
src/DynamoCoreWpf/ViewModels/Preview/InfoBubbleViewModel.cs:524
- There are no tests covering
CopyToClipboardCommand
,CopyTextToClipboard
, orCanCopyToClipboard
. Adding unit tests for both success and failure scenarios will help ensure clipboard logic and analytics tracking remain reliable.
private void CopyTextToClipboard(object parameter)
src/DynamoCoreWpf/Properties/Resources.en-US.resx:450
- The new
<data>
element appears inserted without matching indentation and may have a mismatched closing tag. Verify the XML structure and alignment match existing entries so the resource file parses correctly.
<data name="CopyToClipboardTooltip" xml:space="preserve">
HorizontalAlignment="Right" | ||
VerticalAlignment="Center" | ||
BorderThickness="0" | ||
Command="{Binding ElementName=InfoBubbleWindowUserControl, Path=DataContext.CopyToClipboardCommand}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ElementName binding refers to 'InfoBubbleWindowUserControl', but the root UserControl has no x:Name set. This will break the Command binding at runtime. Consider adding x:Name="InfoBubbleWindowUserControl" on the root or switch to a RelativeSource binding to the view model.
Copilot uses AI. Check for mistakes.
Looks like I need to update the description with release notes, etc when I update the images (failed PR Description check) |
It did not appear that this would be conducive to unit tests, but I could probably use some guidance in that area, including where to add any tests. I think it will be more appropriate to write an AGT test and will do so after the PR is merged. |
Looks like there is an issue with declaring this as part of the API? I will need some guidance there as well. |
…ment/margin of the button
Some minor comments, and I think you can now update the PR description |
Purpose
(FILL ME IN) This section describes why this PR is here. Usually it would include a reference to the tracking task that it is part or all of the solution for.
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Added a button to the InfoBubble to copy the text from the InfoBubble to the clipboard
Reviewers
Copilot
Aparajit
Ashish
Reddy
FYIs
Aaron