Skip to content
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

feat: Copy to clipboard all text nodes as text #5013

Merged
merged 2 commits into from Apr 5, 2022
Merged

Conversation

Fausto95
Copy link
Member

@Fausto95 Fausto95 commented Apr 5, 2022

Kapture 2022-04-05 at 14 56 25

@vercel
Copy link

vercel bot commented Apr 5, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployments, click below or on the icon next to each commit.

excalidraw – ./

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/8pVQvPsV7mqTxtGEdKHxa6M1cCXm
✅ Preview: https://excalidraw-git-copy-all-text-nodes-excalidraw.vercel.app

excalidraw-package-example – ./src/packages/excalidraw

🔍 Inspect: https://vercel.com/excalidraw/excalidraw-package-example/EpP6u8L3CN8hxWq7QwrVgMpwyyu6
✅ Preview: https://excalidraw-package-example-git-copy-all-text-nodes-excalidraw.vercel.app

@ad1992 ad1992 changed the title Copy to clipboard all text nodes as text feat: Copy to clipboard all text nodes as text Apr 5, 2022
@Fausto95 Fausto95 requested review from dwelle and ad1992 April 5, 2022 13:00
Comment on lines 5488 to 5490
if (probablySupportsClipboardWriteText && elements.length > 0) {
options.push(copyAllTextNodesAsText);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should show this option only for text elements

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd show for any selection that contains at least 1 text element.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that is better to show it only when all the elements are text nodes

@@ -9,6 +9,7 @@
"copy": "Copy",
"copyAsPng": "Copy to clipboard as PNG",
"copyAsSvg": "Copy to clipboard as SVG",
"copyAllTextNodesAsText": "Copy to clipboard all nodes as text",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"copyAllTextNodesAsText": "Copy to clipboard all nodes as text",
"copyAllTextNodesAsText": "Copy to clipboard as a single text element",

better renaming?

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, thanks @Fausto95 ❤️

@Fausto95 Fausto95 merged commit 670ceaf into master Apr 5, 2022
@Fausto95 Fausto95 deleted the copy-all-text-nodes branch April 5, 2022 13:31
@dwelle
Copy link
Member

dwelle commented Apr 5, 2022

Actually, this isn't working quite right.

  1. You should be able to copy text of selected elements even if there are other non-text elements on the scene
  2. It should only copy selected elements, not all elements on canvas
  3. The contextMenu action should only work on selection (elements), not on canvas

Haven't reviewed the code itself.

@dwelle
Copy link
Member

dwelle commented Apr 5, 2022

I'd also put \n\n between texts of each element, i.e. make each element into a paragraph. Though this is debatable, I think it makes sense because each text element usually is its own thought, so bunching everything together isn't as useful, especially considering that you wouldn't be able to distinguish between single element line breaks and multiple elements.

@Fausto95
Copy link
Member Author

Fausto95 commented Apr 5, 2022

Actually, this isn't working quite right.

  1. You should be able to copy text of selected elements even if there are other non-text elements on the scene
  2. It should only copy selected elements, not all elements on canvas
  3. The contextMenu action should only work on selection (elements), not on canvas

Haven't reviewed the code itself.

Oh yea, you're right. I'll make a follow up PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants