Clipboard - ability to copy/paste field values in Inspector #1370
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a clipboard feature for copying and pasting field values in the Inspector. The feature allows users to copy values from entity fields and paste them to compatible fields of the same type, with type validation to ensure compatibility between source and destination fields.
- Adds a context menu with copy/paste functionality for Inspector fields
- Implements type-aware clipboard storage using localStorage with type validation
- Integrates clipboard functionality into the attributes inspector with contextmenu handlers
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/editor/storage/clipboard-context-menu.ts | Core clipboard functionality with context menu, type validation, and copy/paste operations |
| src/editor/inspector/attributes-inspector.ts | Integration of clipboard functionality into Inspector fields with contextmenu event handlers |
| src/editor/index.ts | Import statement to register clipboard functionality |
| sass/editor/_editor-clipboard.scss | Styling for clipboard context menu and field highlighting |
| sass/editor.scss | Import for clipboard-specific styles |
| domElement.classList.add('pcui-highlight-flash'); | ||
| setTimeout(() => { | ||
| domElement.classList.remove('pcui-highlight-flash'); | ||
| }, 250); | ||
| }); |
There was a problem hiding this comment.
PCUI has a function Element#flash but I guess some of the UI is still legacy UI....is that why you're having to do this?
There was a problem hiding this comment.
They are different. Element#flash uses outline on element. While it is ok on many elements like button or input, it does not look good on LabelGroup element due to lack of padding. So pcui-highlight-flash class implements highlight with :before class with padding to better outline whole field.
willeastcott
left a comment
willeastcott
left a comment
There was a problem hiding this comment.
Approving with some minor comments/feedback. Nice one!! 🚀
kpal81xd
left a comment
kpal81xd
left a comment
There was a problem hiding this comment.
Overall looks good made some comments regarding styling.
As for the TODOs are these planning to be added in this PR or left for later?
| this._suspendChangeEvt = false; | ||
| this._onAttributeChangeHandler = this._onAttributeChange.bind(this); | ||
|
|
||
| this._clipboardTypes = editor.call('clipboard:types') ?? null; |
There was a problem hiding this comment.
Type editor call return values since type information is lost in event method
There was a problem hiding this comment.
Not sure I actually understand this comment.
I've added above the field: _clipboardTypes: Set<string> | null;, which helps to satisfy this?
|
TODOs will be added as subsequent PR's to keep this PR more focused. As it is already useful to the users. |
|
The newly introduced rule on linter: Making readability ugly. In many cases of simple "return early", now instead of simply: if (!item) continue;It becomes: if (!item) {
continue;
}You guys sure about that? Specifically on this PR, this readable states: let type = attr.type;
if (type === 'select') type = attr.args.type;
if (type === 'assets') type = 'array:asset';
if (type === 'slider') type = 'number';
if ((type === 'asset' || type === 'array:asset') && attr.args?.assetType) type += `:${attr.args.assetType}`;Became this: if (type === 'select') {
type = attr.args.type;
}
if (type === 'assets') {
type = 'array:asset';
}
if (type === 'slider') {
type = 'number';
}
if ((type === 'asset' || type === 'array:asset') && attr.args?.assetType) {
type += `:${attr.args.assetType}`;
}Maybe lets focus on doing features, instead of styling and moving the code around, please? |
|
That rule change was less 5 seconds to implement and even less time/effort to apply automatically so I do not think it takes time away from implementing features |
|
OK, guys - we're good to merge now? 😄 |
Yep all good from me 🫡 |
|
Let's gooOOOooooOOO!!! Merging. Thanks @Maksims! |
|
Thank you for the review! |
3 participants
Fixes #216 #30 #891 #520
Master ticket: #1369
This PR introduces new feature for copy/paste values between various fields.
As fields have a value type (e.g. vec3, asset[], rgba, etc) we can store such info with the value in editor's clipboard (localStorage), and provide pasting capabilities of compatible types.
This is a first iteration, it only supports clipboard when entities are selected. Assets will come a bit later.