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
fix: Copy to clipboard all text nodes as text #5014
Conversation
This pull request is being automatically deployed with Vercel (learn more). excalidraw – ./🔍 Inspect: https://vercel.com/excalidraw/excalidraw/9g4KkkeVUE4QSjidYrjXuuVFVEFs excalidraw-package-example – ./src/packages/excalidraw🔍 Inspect: https://vercel.com/excalidraw/excalidraw-package-example/2ZX4BWU1cT45ptAV73TB8yBr9xQE |
src/components/App.tsx
Outdated
const isTextNodesOnly = selectedElements.every((element) => | ||
isTextElement(element), | ||
); |
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.
Is there actually a downside to allowing mixed elements? For example when you want to copy the whole canvas texts, so that you don't need to select items one by one.
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.
There are no downsides!
That's the use-case I wasn't thinking of, will add it!
const selectedElements = getSelectedElements( | ||
getNonDeletedElements(elements), | ||
appState, | ||
true, |
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.
this should be false
since we are not including bound text elements
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.
but we should include bound text elements?
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.
That's what I thought!
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.
Earlier since we decided to show only for text elements that's where I thought it could cause confusion for bound text, but now we are showing for mixed elements so let's include bound text too
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.
Done!
8e539e9
to
01fb286
Compare
Everything should be working now! |
I completely forgotten about using |
src/actions/actionClipboard.tsx
Outdated
); | ||
|
||
const text = selectedElements.reduce((acc, element) => { | ||
if (element.type === "text") { |
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.
if (element.type === "text") { | |
if (isTextElement(element)) { |
since we have a util already
c3e77e1
to
b26baa4
Compare
|
||
const text = selectedElements.reduce((acc, element) => { | ||
if (isTextElement(element)) { | ||
return `${acc}${element.text}\n\n`; |
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.
shouldn't this be single newline ?
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.
Nope, we actually want to have two newlines so we can distinguish the nodes!
fixes bugs introduced by #5013