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

Convert Oembed Package to Js #6688

Merged
merged 5 commits into from
Apr 24, 2017
Merged

Convert Oembed Package to Js #6688

merged 5 commits into from
Apr 24, 2017

Conversation

MartinSchoeler
Copy link
Contributor

@RocketChat/core Please let me know if i something is wrong, since this is a fairly big package.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-6688 April 13, 2017 19:47 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-6688 April 13, 2017 20:02 Inactive
@engelgabriel engelgabriel modified the milestone: 0.56.0 Apr 18, 2017
if (this.collapsed) {
return this.collapsed;
} else {
return (user && user.settings && user.settings.preferences && user.settings.preferences.collapseMediaByDefault);
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 move the variable declaration here. And there is no need for the parenthesis

if (this.collapsed) {
return this.collapsed;
} else {
return (user && user.settings && user.settings.preferences && user.settings.preferences.collapseMediaByDefault) === true;
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here

loadImage() {
const user = Meteor.user();

if (user && user.settings && user.settings.preferences && user.settings.preferences.autoImageLoad === false && this.downloadImages) {
Copy link
Member

Choose a reason for hiding this comment

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

I guess the correct condition here is this.downloadImages == null

if (user && user.settings && user.settings.preferences && user.settings.preferences.autoImageLoad === false && this.downloadImages) {
return false;
}
if (Meteor.Device.isPhone() && user() && user.settings && user.settings.preferences && user.settings.preferences.saveMobileBandwidth && this.downloadImages) {
Copy link
Member

Choose a reason for hiding this comment

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

User is not a function here, you already called the function to get the data. The correct condition for downloadImage is this.downloadImages == null

if (this.collapsed != null) {
return this.collapsed;
} else {
return (user && user.settings && user.settings.preferences && user.settings.preferences.collapseMediaByDefault) === true;
Copy link
Member

Choose a reason for hiding this comment

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

Move the declaration here and remove the parenthesis

if (this.collapsed != null) {
return this.collapsed;
} else {
return (user && user.settings && user.settings.preferences && user.settings.preferences.collapseMediaByDefault) === true;
Copy link
Member

Choose a reason for hiding this comment

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

Move the declaration here and remove the parenthesis

if (this.collapsed) {
return this.collapsed;
} else {
return (user && user.settings && user.settings.preferences && user.settings.preferences.collapseMediaByDefault) === true;
Copy link
Member

Choose a reason for hiding this comment

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

Same here

if (this.collapsed) {
return this.collapsed;
} else {
return (user && user.settings && user.settings.preferences && user.settings.preferences.collapseMediaByDefault) === true;
Copy link
Member

Choose a reason for hiding this comment

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

Same here

};

const getUrlContent = function(urlObj, redirectCount, callback) {
if (redirectCount == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Use default value here

});
}
if (content && content.statusCode !== 200) {
return;
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 forget to return the data? return data;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rodrigok But here data would just return undefined since the variable is being created just after that return, or did i miss something here?

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-6688 April 20, 2017 14:37 Inactive
@MartinSchoeler
Copy link
Contributor Author

@rodrigok could you do another review?

@rodrigok rodrigok merged commit c1532db into develop Apr 24, 2017
@rodrigok rodrigok deleted the oembed-to-js branch April 24, 2017 17:57
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.

3 participants