Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added document tool to upload images #42
base: master
Are you sure you want to change the base?
Added document tool to upload images #42
Changes from 13 commits
e6a5639
6a4c9bd
4b1ee6e
9f210b3
38bebf3
e8ff46c
5247853
0a99ad2
061f84d
bc0e1e9
b1b875a
4e258c0
cd8a9ca
0067399
79352c9
8aefd41
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
We should probably use jpeg by default here... We can make it configurable, though.
https://caniuse.com/#feat=webp
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.
jpeg does not support transparency, which would require us to check what kind of image was uploaded and if it made use of the alpha channel.
If it is a png with transparency we would not be able to compress it other than by shrinking the dimensions.
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.
I would suggest using a webp polyfill, e.g. https://github.com/chase-moskal/webp-hero
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.
Let's maybe start with JPEG, and then add support for webp, with a polyfill, in a second pull request ? webp-hero won't work as-is with data urls...
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 uses the newly reduced value for
scale
. I think it was meant to use the previous value, which is the value used withdrawImage()
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.
We should probably have a maximum and a minimum image size, and scale only images that do not fit.
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.
Should this be
boardData.existingDocuments
?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.
Large images failed for me, causing a transport error. The socket.io default maxHttpBufferSize is 1e6, so I changed this to 1e6 - 500.
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 probably be handled in the BoardData class
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.
Also, we should also verify the provided URL, to check that it's a data URL.
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.
we would still broadcast the large files to all users in that case
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.
Yes, you are right, I hadn't thought about that. I still think the board-specific code should happen in BoardData, but we should probably set the
httpBufferSize
option of socket.io in addition, to avoid having to handle and dispatch large messages.