-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Comment mentions with spaces #11836
Comment mentions with spaces #11836
Conversation
The problem at the moment is, that the server/apps/comments/js/commentstabview.js Lines 424 to 430 in 52723f0
In Talk we do the replacement in php with a simple str_replace and therefor do not have this problem. So my idea would be to screw the word end check for quoted user ids. |
52723f0
to
cd65fa1
Compare
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.
Would you mind adding another test case to \Test\Comments\CommentTest::mentionsProvider()
that supports the uid-with-space-scenario?
Otherwise, the code looks good and i like the simplicity
var $inserted = $this.parent(); | ||
$inserted.html('@' + $this.find('.avatar').data('username')); | ||
var $this = $(this), | ||
$inserted = $this.parent(), |
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.
Imho this style is rather confusing than improving readability. Might be just me tho.
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 wanted to here this, before diving into tests, because if we don't agree on |
@MorrisJobke this is not retro-active, because the saved comment is different (old: |
Signed-off-by: Joas Schilling <[email protected]>
Signed-off-by: Joas Schilling <[email protected]>
cd65fa1
to
375589b
Compare
Added a unit test and rebased. still working for me, so ready for review. |
Ah - I created the comment first and then changed the branch. Let me test again. |
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.
Tested and works 👍
Unrelated trash bin failure -> merge |
foo bar
Before:
Hi {{@Unknown user}} bar test
After:
Hi {{@User with space in ID}} test
Fix #2443