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

after pasting images, upload them if storageType isn't base64 #56

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

webzwo0i
Copy link

@webzwo0i webzwo0i commented Sep 29, 2021

This is a fix for #55
After pasting images, in case storageType is not base64, the image is immediately removed from DOM and uploaded via POST.

~~The upload code is copied from toolbar.js - should this be moved into an separate function to avoid duplication?

Any feedback welcome. I mark this as draft, because I didn't explicitly test the format of lineAttributes.img, so the new code is executed even when the toolbar button is used (that would be wrong). However, everything seems to work which is kind of unexpected.~~

@webzwo0i
Copy link
Author

webzwo0i commented Nov 16, 2021

adjusted the tests: if storageType is local it will check if an image element appears.

There are two additional problems that seem to appear in the tests:

  • an additional new line is introduced after the image added a comment that this is due to ace.ace_returnKey
  • the image seems to larger when it's embedded via link can't reproduce anymore

The old tests check if the uploaded SVG was included in the HTML,
immediately after setting the HTML. In case Etherpad would remove the
image due to some error it's possible that the test returns true even if
the image gets removed in the end. To make this a little less likely,
prior to checking the HTML, let's wait for an ACCEPT message from the
server.
@webzwo0i webzwo0i force-pushed the support-pasting-images branch from 4f8d188 to 9dd7b62 Compare November 28, 2021 02:53
@webzwo0i webzwo0i marked this pull request as ready for review November 28, 2021 02:54
@webzwo0i webzwo0i force-pushed the support-pasting-images branch from 9dd7b62 to 0daf766 Compare November 28, 2021 03:05
@webzwo0i
Copy link
Author

webzwo0i commented Nov 28, 2021

This is ready now. @ilmartyrk @rhansen @JohnMcLear Can you give it a try? Any feedback welcome

@webzwo0i
Copy link
Author

I think I need to look at the regex again. It seems to work, but it throws some lint error

@webzwo0i webzwo0i force-pushed the support-pasting-images branch from 0daf766 to 5c5c4b4 Compare November 28, 2021 14:22
@webzwo0i
Copy link
Author

Now slightly less readable without named groups in the regex, but it should work

Instead of observing the clipboard, we first put the data URI into the
pad text, so it is uploaded by contentCollector.
When Etherpad starts using the Clipboard API we should use it too.
@webzwo0i webzwo0i force-pushed the support-pasting-images branch from 5c5c4b4 to af2b141 Compare November 29, 2021 17:26
@webzwo0i
Copy link
Author

npm run lint should pass now

@tiblu
Copy link
Member

tiblu commented Dec 3, 2021

@webzwo0i Thanks a lot for your contribution. We have not missed your PR but we have time issues on Citizen OS side and haven't had opportunity to look at it. We will snooze this to unknown times from our side.
If @JohnMcLear has an opportunity earlier, it would be great, but if not, this has to wait for a little.
Sorry, but that this is the way it is right now.

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.

2 participants