-
-
Notifications
You must be signed in to change notification settings - Fork 57
Add autocomplete to comments/announcements #246
Conversation
import 'jquery-textcomplete'; | ||
|
||
export function autocompleteUsers(selector, users) { | ||
console.log(users); |
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.
do we need to leave this in?
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.
Accident!
I like it... any chance for an animated gif in the OP? |
I'm not sure what's with it being offset like that. Maybe a style issue? |
User | ||
|> Repo.all | ||
|> Enum.map(&format_user_json/1) | ||
|> Poison.encode! |
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 think we should try to keep calls to repo in the controller. I wouldn't expect the view to load the users. What do you think about having this accept (users)
?
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 felt the same way, but also conflicted. Is there any good reason not to do this?
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.
It's just a ton easier not to worry about it in the controller, and the view, and the template.
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.
If I'm trying to figure out where a slow query is coming from I would have a hard time figuring out that it was from here. Or imagine if we ever wanted to Saasify Constable, we would likely not notice this call since it's not in the controller, and then we'd return all users.
I don't think we're going to productize Constable soon, but I think it illustrates the potential problems that putting it in the view can cause
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'm not sure if I agree that we would have a hard time trying to find it. We'd check the whole stack and find it being called in the template.
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.
In the Phoenix book and in blog posts they say to keep views pure, I think that's what most Elixir devs would expect. Not sure if that helps clarify anything or not
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.
MentionFinder
above also breaks it. 😄
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.
Thoughts on that one?
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.
My thought would be that we remove the calls to the Repo
in MentionFinder
and instead pass it a list of users or usernames that it can check against. That would actually be faster and would make things a bit more clear. Though I think that could be done in another PR and isn't urgent. I also think that service objects are generally ok, so even if we left it as-is I'd feel a bit better about that
In general, if you can keep queries in the controller, I think that's usually best
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.
To be honest keeping it out of the view feels messier. This is one of those things I thought was terrible in Rails and is terrible here. 😛
I think this LGTM, the offset works for now since it's not in the way of typing |
|
||
export function autocompleteUsers(selector, users) { | ||
$(selector).textcomplete([{ | ||
match: /(^|\s)@(\w*)$/, |
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.
Maybe extract this regex to a named Const
? Also it seems like a different regex than the one on the backend. Could/should they be the same?
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.
This is checking when to try and autocomplete so it has to be different.
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.
Oh ok! Either way I think extracting to a const
would be a good idea
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.
Agreed.
A few small comments, but overall this looks great! |
|> Markdown.to_html | ||
end | ||
|
||
defp bold_usernames(users, text) do |
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.
Should this be duplicated?
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.
It should not! Good catch.
b063a9e
to
23b6d3e
Compare
e852110
to
895dbdd
Compare
This adds user autocompletion to announcements and comments as well as bolding usernames of users who were mentioned.
895dbdd
to
213e6f3
Compare
Fixed the offset issue by downgrading to 0.8.0 from 1.3.0. Weird but works. |
This looks awesome! I'm excited to give this a try :) |
This adds autocompletion of usernames in comments and announcements.
This also bolds @mentioned usernames in comments and announcements when
that user exists.