Skip to content

Issue #24#30

Open
forsd wants to merge 3 commits into
ankitjain28may:masterfrom
forsd:Issue_#24
Open

Issue #24#30
forsd wants to merge 3 commits into
ankitjain28may:masterfrom
forsd:Issue_#24

Conversation

@forsd

@forsd forsd commented Oct 14, 2017

Copy link
Copy Markdown

No description provided.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 56.29% when pulling 4774d5c on forsd:Issue_#24 into eaac03b on ankitjain28may:master.

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd Are you able to set up this project on your system ?

@forsd

forsd commented Oct 14, 2017

Copy link
Copy Markdown
Author

Sure.

@ankitjain28may

Copy link
Copy Markdown
Owner

Is it working perfectly on your system ?

@forsd

forsd commented Oct 14, 2017

Copy link
Copy Markdown
Author

Register, login, chat working. No errors occured.

@ankitjain28may

Copy link
Copy Markdown
Owner

Anything that isn't working, You can open issues here, I'll check out the PR tomorrow, Thanks :)

@forsd

forsd commented Oct 17, 2017

Copy link
Copy Markdown
Author

Will you merge?

@ankitjain28may

ankitjain28may commented Oct 17, 2017

Copy link
Copy Markdown
Owner

@forsd Yes, Actually I am unable to check it out, will surely check it asap.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.7%) to 55.573% when pulling 91c8c83 on forsd:Issue_#24 into eaac03b on ankitjain28may:master.

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd Is this PR contains 'Show typing issue' ?

@forsd

forsd commented Oct 20, 2017

Copy link
Copy Markdown
Author

No. This contains only youtube video embedding. The second contains typing issue.

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd, This also contains the Typing commits check out the commits..

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd I have checked, Youtube embedding is working great but typing isnt working.. so either remove the lastest(typing) commit so i will merge so fixed typing as well

var messageText = $("<div></div>").addClass("message-text");

// Embed YouTube video if link presents in message.
var youtube_link = data[i].message.indexOf('https://www.youtube.com');

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

@forsd You need to check it for youtube short url like https://youtu.be/* and one more thing, pls put this link in <a> tag so user can opn this link directly in the new tab also

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

OK will do.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Please check it.

@forsd

forsd commented Oct 20, 2017

Copy link
Copy Markdown
Author

Typing removed from this PR.

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd I have requested for some changes.

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd Are you fixing this ?

@forsd

forsd commented Oct 24, 2017

Copy link
Copy Markdown
Author

Sure.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.5%) to 56.774% when pulling dad5312 on forsd:Issue_#24 into eaac03b on ankitjain28may:master.

// Embed YouTube video if link presents in message.
var result = anchorme(data[i].message.replace(/<(?:.|\n)*?>/gm, ''), { attributes:[ { name:"target", value:"_blank" } ] });
var youtube_link = data[i].message.indexOf('https://www.youtube.com');
if(youtube_link !== -1) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Rather repeating the same code again and again, it would be better to make a new function and cal that function on getting link similar to http://youtube.com or http://youtu.be/ else will send message. What do you say ??

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd Are you going to fix this ?

@forsd

forsd commented Nov 2, 2017

Copy link
Copy Markdown
Author

@ankitjain28may Sure I will

@ankitjain28may

Copy link
Copy Markdown
Owner

@forsd Are you still working ?

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