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

Link and color nicks mentioned in messages #1709

Merged
merged 1 commit into from Nov 27, 2017

Conversation

Projects
None yet
6 participants
@MaxLeiter
Member

MaxLeiter commented Nov 14, 2017

Not sure how I like this. kiwi does it. Can disable in settings (but requires reload to change sent messages, which is fine imo). Setting probably needs better wording.

Should be good to go.

screen shot 2017-11-14 at 6 50 54 pm

relies on #1712

@fnutt

This comment has been minimized.

Show comment
Hide comment
@fnutt

fnutt Nov 14, 2017

Contributor

Big thumbs up for this!

Contributor

fnutt commented Nov 14, 2017

Big thumbs up for this!

@MaxLeiter

This comment has been minimized.

Show comment
Hide comment
@MaxLeiter

MaxLeiter Nov 15, 2017

Member

I think this is good to go

Member

MaxLeiter commented Nov 15, 2017

I think this is good to go

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Nov 15, 2017

Member

This is a very fine addition, I haven't tested but love it already 👏 👏 Awesome job @MaxLeiter!

Last time this was attempted (client-side only), @xPaw has some performance concerns. The way you did it seems to me like you do not have this issue anymore. I'll let @xPaw give a first pass at this for this reason.

Member

astorije commented Nov 15, 2017

This is a very fine addition, I haven't tested but love it already 👏 👏 Awesome job @MaxLeiter!

Last time this was attempted (client-side only), @xPaw has some performance concerns. The way you did it seems to me like you do not have this issue anymore. I'll let @xPaw give a first pass at this for this reason.

@astorije astorije requested review from astorije and xPaw Nov 15, 2017

@MaxLeiter

This comment has been minimized.

Show comment
Hide comment
@MaxLeiter

MaxLeiter Nov 15, 2017

Member

I'm all for no setting 👍

Member

MaxLeiter commented Nov 15, 2017

I'm all for no setting 👍

@xPaw

xPaw approved these changes Nov 16, 2017

@xPaw xPaw added this to the 2.7.0 milestone Nov 16, 2017

@xPaw xPaw added the Type: Feature label Nov 16, 2017

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Nov 19, 2017

Member

@MaxLeiter, could you rebase on master please? 

Member

astorije commented Nov 19, 2017

@MaxLeiter, could you rebase on master please? 

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Nov 19, 2017

Member

@MaxLeiter, since #1712 was merged, I took the liberty to rebase that for you and fix conflicts (which were just dangling comma shit) so I can test it with most recent master.

Member

astorije commented Nov 19, 2017

@MaxLeiter, since #1712 was merged, I took the liberty to rebase that for you and fix conflicts (which were just dangling comma shit) so I can test it with most recent master.

Show outdated Hide outdated src/models/msg.js
@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Nov 19, 2017

Member

@xPaw, did you see this? https://travis-ci.org/thelounge/lounge/jobs/304356874#L2268
I'm getting this when testing this PR locally as well.

Member

astorije commented Nov 19, 2017

@xPaw, did you see this? https://travis-ci.org/thelounge/lounge/jobs/304356874#L2268
I'm getting this when testing this PR locally as well.

@astorije

Works really well, I love it! Hopefully we can fix this breaking test soon :/

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Nov 20, 2017

Member

@MaxLeiter, @xPaw, I believe I have something to fix the failing test. I need to play more with that though to make it merge-ready. I'll try to do that after work tonight. Thus ignore it for now.

Member

astorije commented Nov 20, 2017

@MaxLeiter, @xPaw, I believe I have something to fix the failing test. I need to play more with that though to make it merge-ready. I'll try to do that after work tonight. Thus ignore it for now.

@MaxLeiter

This comment has been minimized.

Show comment
Hide comment
@MaxLeiter

MaxLeiter Nov 20, 2017

Member

@Bonuspunkt could you take a look? This is broken with nicks, colors, bold, etc

Member

MaxLeiter commented Nov 20, 2017

@Bonuspunkt could you take a look? This is broken with nicks, colors, bold, etc

@Bonuspunkt

This comment has been minimized.

Show comment
Hide comment
@Bonuspunkt

Bonuspunkt Nov 20, 2017

Contributor

i'm guessing rel #1718 ?
what are the expected results?

Contributor

Bonuspunkt commented Nov 20, 2017

i'm guessing rel #1718 ?
what are the expected results?

@MaxLeiter

This comment has been minimized.

Show comment
Hide comment
@MaxLeiter

MaxLeiter Nov 21, 2017

Member

Right now duplicates are produced (www.MaxLeiter.com with nick MaxLeiter in the channel renders www.MaxLeiter.comMaxLeiter. Each word is correct (link is a link, nick is nick))

Member

MaxLeiter commented Nov 21, 2017

Right now duplicates are produced (www.MaxLeiter.com with nick MaxLeiter in the channel renders www.MaxLeiter.comMaxLeiter. Each word is correct (link is a link, nick is nick))

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Nov 27, 2017

Member

FYI, just tested it works fine with copy/paste. Can't wait for this feature!

Member

astorije commented Nov 27, 2017

FYI, just tested it works fine with copy/paste. Can't wait for this feature!

@astorije

Works great! I'll let xPaw give a second review

@xPaw

This comment has been minimized.

Show comment
Hide comment
@xPaw

xPaw Nov 27, 2017

Member

@MaxLeiter Can you add some tests for this? Like http://example.com with example being an user, and some others?

Member

xPaw commented Nov 27, 2017

@MaxLeiter Can you add some tests for this? Like http://example.com with example being an user, and some others?

@xPaw xPaw merged commit 4e2eed2 into thelounge:master Nov 27, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
input: "test, MaxLeiter",
expected:
"test, " +
"<span role=\"button\" class=\"user color-12\" data-name=\"MaxLeiter\">" +

This comment has been minimized.

@astorije

astorije Nov 27, 2017

Member

Can you use single quotes instead to avoid escaping? (See my PR I just merged)

@astorije

astorije Nov 27, 2017

Member

Can you use single quotes instead to avoid escaping? (See my PR I just merged)

This comment has been minimized.

@xPaw

xPaw Nov 27, 2017

Member

This was merged before yours. You should have enforced this with eslint rule by the way.

@xPaw

xPaw Nov 27, 2017

Member

This was merged before yours. You should have enforced this with eslint rule by the way.

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Nov 27, 2017

Member
Member

astorije commented Nov 27, 2017

@astorije astorije changed the title from Link nicks mentioned in messages to Link nicks mentioned in messages Jan 14, 2018

@astorije astorije changed the title from Link nicks mentioned in messages to Link and color nicks mentioned in messages Jan 16, 2018

@xPaw xPaw removed their assignment Mar 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment