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

[NEW] Image preview as 32x32 base64 jpeg #9218

Merged
merged 15 commits into from
Feb 14, 2018

Conversation

jorgeluisrezende
Copy link
Contributor

@RocketChat/core

Open #5563

Added a image preview as small base64 50x50 pictures embedded on the msg object.

I just made a function, and had call it on the sendFileMessage method, and i fixed the build errors.
I open another PR because my old branch was in conflict, at this time i did some build tests.

@jorgeluisrezende
Copy link
Contributor Author

@rodrigok, now it is all ok!

Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Let's convert this into using promises and using Promise.await, that way the sendFileMessage is has less callbacks being passed around.


return msg;
return msg;
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong, now the method will have nothing returned

attachment.image_size = file.size;
if (file.identify && file.identify.size) {
attachment.image_dimensions = file.identify.size;
Meteor.wrapAsync(FileUpload.resizeImagePreview(file, Meteor.bindEnvironment(function(base64Preview) {
Copy link
Member

Choose a reason for hiding this comment

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

You could check if file is image and call this only for images, makes more sense to me.

} else {
callback();
}
},
Copy link
Member

Choose a reason for hiding this comment

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

You can create a new method like resizeImagePreviewSync that does not receive the callback and do the wrapAsync inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will do that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to do that, but the file does not finish to send, might i'm doing something wrong.
I think the Bradley suggestion that uses Promises is nice, but i don't know if you like that..
What do you think?

@RocketChat RocketChat deleted a comment Dec 27, 2017
@@ -9,7 +9,6 @@ renderMessageBody = function(msg) {
}

const message = RocketChat.callbacks.run('renderMessage', msg);

Copy link
Member

Choose a reason for hiding this comment

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

Do not introduce unnecessary changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, it was a mistake when i was making tests..


const transformer = sharp().resize(50, 50).max().toBuffer(function(err, out) {
if (err) { reject(err); }
Copy link
Member

Choose a reason for hiding this comment

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

Without a return you will reject and resolve the promise right?

@@ -39,6 +39,7 @@ Meteor.methods({
if (file.identify && file.identify.size) {
attachment.image_dimensions = file.identify.size;
}

Copy link
Member

Choose a reason for hiding this comment

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

You could execute the resize here using await to keep the flow sync


Meteor.defer(() => RocketChat.callbacks.run('afterFileUpload', { user, room, message: msg }));

return msg;

Copy link
Member

Choose a reason for hiding this comment

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

Do not introduce unnecessary changes

@jorgeluisrezende
Copy link
Contributor Author

@rodrigok, now i guess it is ok.

title_link: fileUrl,
title_link_download: true
};
(async function() {
Copy link
Member

Choose a reason for hiding this comment

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

Did you try to remove this wrapper? AFAIK all meteor methods are async by default allowing you to call await inside.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had tested a lot of ways to do it, when i tryed to call await without the wrapper it was failing in the build...

@rodrigok rodrigok added this to the 0.62.0 milestone Jan 30, 2018
# Conflicts:
#	package-lock.json
#	package.json
@rodrigok rodrigok dismissed stale reviews from graywolf336 and themself February 12, 2018 19:02

Done

@rodrigok rodrigok changed the title [NEW] Image preview as 50x50 base64 obj [NEW] Image preview as 32x32 base64 jpeg Feb 12, 2018
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

should we start using image_preview already?

@rodrigok
Copy link
Member

@sampaiodiego It is mainly for mobile guys right now, once merged I'll create a new issue for UI guys to start using it.

@rodrigok rodrigok merged commit 20db19b into RocketChat:develop Feb 14, 2018
@rodrigok rodrigok mentioned this pull request Feb 28, 2018
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.

4 participants