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.

Sign up for GitHub

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

Jump to bottom

[WIP] (please do not merge yet) Add autocompleting usernames with @ symbol #2913 #2925

Merged
merged 18 commits into from
Jul 21, 2018
Merged

[WIP] (please do not merge yet) Add autocompleting usernames with @ symbol #2913 #2925

merged 18 commits into from
Jul 21, 2018

Conversation

cheneyshreve01
Copy link
Collaborator

@cheneyshreve01 cheneyshreve01 commented Jun 26, 2018 •
edited by jywarren
Loading

Hi @publiclab/reviewers, @ViditChitkara

I was able to get this working using jquery's atwho library. I was not able to get it to work using the emoji autocomplete approach. If you can give me more details on that and prefer to have it implemented differently, I'm happy to revise this. I do think I can get the Emoji approach to work with the hashes (I was planning on opening a different issue for that) because you can use the hash emoji, so that one should be very similar to the smiley emojis.

The reason I've marked this PR as WIP is because, when I did an upstream fetch and rebase of the master (which is not reflected in this current branch), I get the error

ActionView::Template::Error (couldn't find file 'noty/lib/noty.js' with type 'application/javascript'

Checked in these paths:
Even though the appropriate files seem to be in-place:

app/assets/javascripts/application.js
//= require noty/lib/noty.js
//= require noty_notification.js
//= require jquery-confirm/js/jquery-confirm.js

And I have run bundle update, bundle install, and restarted the server. Any idea why this error still persists or have you gotten this error? I'm not sure if you can still merge this PR with the branch being behind master because I have not rebased this branch per the error above.

here is the demo of how the current branch/changes work to see if you want to keep it or maybe have me do some revisions, thanks so much for your help.

demo_atwho

@ViditChitkara
Copy link
Member

@cheneyshreve try bower install noty and then restart your server. The required "noty" dependencies might not be fetched, since they are public/assets (which is in gitignore). Doing bower install noty would download it in your public/assets folder. Hope this helps.

@cheneyshreve01
Copy link
Collaborator Author

@ViditChitkara Thank you!! Bower install noty did the trick.

@ViditChitkara
Copy link
Member

here is the demo of how the current branch/changes work to see if you want to keep it or maybe have me do some revisions, thanks so much for your help.

The autocomplete for usernames is working good I guess. However atwho's default styling is being overridden. Can you explore something about atwho's manual styling? @jywarren any ideas on this??

@cheneyshreve01
Copy link
Collaborator Author

Hi @ViditChitkara, @jywarren . @ViditChitkara thanks for the tip on noty, I was able to fetch upstream and rebase changes for this branch. I will look into styling and see if I can find anything, but yes, let me @jywarren if you have any ideas about better styling. Thanks both :)

@plotsbot
Copy link
Collaborator

plotsbot commented Jun 26, 2018 •
edited
Loading

1 Warning
⚠️ It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!
2 Messages
📖 @cheneyshreve Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 Pull Request is marked as Work in Progress

Generated by 🚫 Danger

@cheneyshreve01
Copy link
Collaborator Author

Hi @ViditChitkara, @jywarren I had not yet added atwho to application.css. now the styling works as intended :)
demo_atwho

@ViditChitkara
Copy link
Member

ViditChitkara commented Jun 26, 2018 via email

@jywarren
Copy link
Member

Hi! I'm wondering if it could be solved in a way very similar to @ViditChitkara's solution using the horsey library:

https://github.com/cheneyshreve/plots2/blob/74e894ff92813ce6d7b630c6423768ee72440882/app/views/comments/_form.html.erb#L78-L86

I think you can likely refactor your code just a little bit so that we have a similar solution. Do you think that would be OK? I love your implementation though!

One thing I worry a bit about is autocompleting any user name from the whole database -- we have a lot of users (like, 300k+)! Instead, we might use the API to narrow the results:

https://github.com/publiclab/plots2/blob/master/doc/API.md

For example you might start typing @jywarren with https://publiclab.org/api/srch/profiles?srchString=jy

The Horsey library is here: https://bevacqua.github.io/horsey/

This is looking really good! Thanks for tackling it -- i'm happy to offer more suggestions too!

@cheneyshreve01
Copy link
Collaborator Author

cheneyshreve01 commented Jun 26, 2018 •
edited
Loading

@ViditChitkara, @jywarren ahh, I think see what you mean now. I originally thought I couldn't use the Emoji approach because I couldn't find the @ for emoji, but it looks like I should be able to find a solution using just the horsey library and plucking the usernames, if I'm interpreting this right, and also limit the dropdown of users by setting a limit and/or looking into the API approach you mention above. I will get to work on the revision, thanks to both of you for the feedback :)

@jywarren
Copy link
Member

jywarren commented Jun 26, 2018 via email

@cheneyshreve01
Copy link
Collaborator Author

hi @jywarren
I'm trying to get the horsey textarea example from the demo site to work, they give the code below at https://bevacqua.github.io/horsey/

horsey(document.querySelector('textarea'), {
source: [{ list: [
{ value: '@michael', text: 'Michael Jackson' },
{ value: '@jack', text: 'Jack Johnson' },
{ value: '@ozzy', text: 'Ozzy Osbourne' }
]}],
getText: 'text',
getValue: 'value',
anchor: '@'
});

I first tried adding that code snippet to /comments/_form.html.erb, checking both 'textarea' and 'textarea#text-input' to see if I typed in the @, if it would populate the input list as it does in the demo, but it doesn't. Also, when that code snippet is added, the emoji autocomplete stops working.

I also tried to edit the one above to reflect the actual username data, using the code below:
/application_helper.rb

def user_names_list
usernames = User.all.collect(&:username)
list = []
usernames.each do |name|
val = "@#{name}"
list << { value: val, text: name }
end
[{ list: list }]
end

Returns this running locally, so it's the same format as the textarea output on the horsey demo site.

[{:value=>"@admin", :text=>"admin"}, {:value=>"@moderator", :text=>"moderator"}, {:value=>"@user", :text=>"user"}, {:value=>"@cheneyshreve", :text=>"cheneyshreve"}]

And to render:
/comments/_form.html.erb

<% usernames = user_names_list %>
horsey(document.querySelector('textarea#text-input'), {
  source: <% usernames %>,
  getText: 'text',
  getValue: 'value',
  anchor: '@',
  limit: 10
});

But no success on that either yet. Any ideas? Thanks for any feedback you can give :)

@jywarren
Copy link
Member

jywarren commented Jun 27, 2018 via email

@cheneyshreve01
Copy link
Collaborator Author

Hi @ViditChitkara, @jywarren Unfortunately, I'm still unable to get things working with horsey. I doubled back on atwho just to see if I could do both the autocomplete username and autocomplete tags without interfering with emoji, and it's easy to add multiple callouts with a configuration setting. I'll double back on horsey again too ( should be same case there I'd imagine) and see if I can figure that out, just haven't had luck with that yet. I was hoping playing around with the other library might give me insight into horsey.

demo_atwho_v2

with code to ensure limits on dropdown

    names = <%= raw User.pluck(:username).compact.to_json %>;
    tagnames  = <%= raw Tag.pluck(:name).compact.to_json %>;
    var mentions_config = {
      at: "@",
      data: names,
      limit: 6,
      tpl: "<li data-value='@${names}'>${names}</li>"
      },
      hashtags_config = {
      at: "#",
      data: tagnames,
      limit: 6,
      tpl: "<li data-value='#${tagnames}'>${tagnames}</li>"
     };

    $('textarea#text-input').atwho(mentions_config).atwho(hashtags_config);

@jywarren
Copy link
Member

jywarren commented Jun 27, 2018 via email

@cheneyshreve01
Copy link
Collaborator Author

@jywarren sure thing, I'll see if I can get atwho working on a page like/post with the rich editor. It's interesting to learn about this, especially with these kinds of editors becoming pretty popular.

@jywarren
Copy link
Member

jywarren commented Jun 27, 2018 via email

@cheneyshreve01
Copy link
Collaborator Author

Hi @ViditChitkara @jywarren Looks like it does work in the rich text editor if I am testing this properly (I added earlier code snippet to rich.html.erb)

testing_atwho_richtexteditor

I think that it might be possible to also use atwho to do the emojis, building on @ViditChitkara's code. I found this code snippet https://gist.github.com/conan007ai/9503021#file-gistfile1-txt-L44 but it is not as sophisticated as @ViditChitkara's because it doesn't do the substitution of the :: around the image so I'm not sure if it would work with markdown. I can try it out if you want. I'd probably need to build on @ViditChitkara's code he has there already. What do you think?

@cheneyshreve01
Copy link
Collaborator Author

Hi @jywarren I checked out the rich text editor and markdown editor and it is possible to use atwho to do autocomplete for the usernames, hashtags, and emojis together. Just let me know if you want me to make issues/PRs to move forward with this.

in the markdown editor

testing_atwho_emoji_hash_username

in rich text editor

testing_atwho_emoji_hash_username_richtext

@jywarren
Copy link
Member

OMGGGGG This is the BEST 🎉 🎉 🐴

Wow. Ok, how about this. For this PR, let's use atwho. If you think it's ready to go, that's great.

Next, let's refactor the emoji one into atwho, so it's consistent.

Third, let's go into the rich editor and implement that feature over there too! That'll be a little more complex but I can help you out. How does that sound?

Amazing work, @cheneyshreve !!! We're so lucky to have your contributions!!!!!

🙌🙌🙌 ⚡️

@jywarren
Copy link
Member

Let's test one more thing -- in the rich text editor, can you switch the mode using the Rich button to bottom right, and see what happens when you keep autocompleting? We may have to "reconstruct" atwho for both modes. But it should be OK.

@cheneyshreve01
Copy link
Collaborator Author

Hi @jywarren many thanks for your support, I'm very excited to get to contribute to this project! Regarding toggling the rich text button & your earlier comments. Based on testing in rich.html.erb, you are right, it will need a bit of refactoring. It does okay with keeping the hashes separate from markdown (e.g. one hash is header 1) vs. Rich text (it keeps it as a tag, yay!) but it identifies a bogus username as a real username if it follows the @ symbol (see gif below) so will need to be refactored in the users controller, maybe adding a if !@user.nil? statement. Or maybe make it fancier and give the user a message that that person is not in the database, and then we'd still need a way to make sure it does not try to link to a bogus user page from the user's controller.

Let me know how you'd like me to proceed. I've refactored the atwho calls into one statement, but obviously we'll want to fix this first.

Editor behavior tests
atwho_editor_tests

@jywarren
Copy link
Member

We can break out and address these issues, indeed. Let's make a new issue for the rich editor! In the meantime, is this PR ready to go? What follow-up issues do we want to make? Let's make a /checklist/ ✅✅✅✅ 😄

jywarren
jywarren reviewed Jun 29, 2018
View reviewed changes
config/database.yml.sqlite Outdated
@@ -0,0 +1,11 @@
development:
Copy link
Member

Choose a reason for hiding this comment

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

Ah, i think this has to be removed -- see this for some help with that!

https://stackoverflow.com/questions/215718/reset-or-revert-a-specific-file-to-a-specific-revision-using-git

👍

Copy link
Member

Choose a reason for hiding this comment

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

Looking great - can you try removing this file from the PR? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Ah, still this one file to be removed!

@cheneyshreve01
Copy link
Collaborator Author

Hi @jywarren the current PR [WIP] has the 3 autocomplete commands (username, tag, emoji) chained in the comments _form.html.erb, which I tested again locally to confirm however I still have some questions.

  • This code works, but as the gif above shows, the markdown editor interprets anything following @ as a username and if a user clicks on that later on, it will throw an error from the user's controller.
  • I have deleted the earlier emoji code (only from the comments form, as I believe the helpers within application_helper for emoji are currently being used elsewhere).
  • Tests pass locally, but due to earlier commit you mention, perhaps I still need to do some git-related revisions? It might be easier to delete this branch and just make a new one since it's not a lot of code. I was following a guide on open source, but perhaps not the best one, or probably I misinterpreted or missed a step.

In terms of a checklist

  • where are we dealing with potential username error? (In this PR, or elsewhere?)
  • open a new issue for adding to Rich text editor
  • (perhaps) open a new issue to determine if other parts of emoji code could be refactored

thanks again for your patience and support :)

@jywarren
Copy link
Member

jywarren commented Jul 1, 2018

I think we can solve the non-existent username issue in another, and it'll be fine, thanks! And the git mergability looks perfect. I'm going to request a review from someone else and then we should be good!

Let's also open a new issue to see if we can optimize the emoji code - especially putting all emoji into a separate file which the browser can cache since it's a lot of code for each page load.

jywarren
jywarren requested changes Jul 1, 2018
View reviewed changes
app/views/comments/_form.html.erb Outdated
},
limit: 6

names = <%= raw User.pluck(:username).compact.to_json %>;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, so here, does atwho allow for remote loading of suggestions and especially ones base on what letters you've already typed? That would do a lot to optimize this for when it runs on the full db which has 300k usernames.

Copy link
Member

Choose a reason for hiding this comment

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

So for this, see my note above about the API and

https://publiclab.org/api/srch/profiles?srchString=jy

I think atwho will probably have a way to fetch remote results!

Thanks and great work!

Copy link
Collaborator Author

@cheneyshreve01 cheneyshreve01 Jul 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @jywarren awesome to learn how to deal with this kind of transaction for a large user community (I haven't run into that before now ;) I think it should be possible to fetch remote data (e.g. from atwho docs):

When you set data as URL string, At.js will launch Ajax request to the server to get the JSON data

   $('textarea').atwho({
        at: "@", 
        data: "http://www.atjs.com/users.json",
        limit: 7
    });

so I would need to combine that with the API you mention to get the proper url.
And other example below where a user is doing more customized calls, e.g. binding an ajax call with "focus" and trigger the atwho on success so that the ajax calls are reduced and the filter gets done locally. Not sure if that is feasible for this situation but will keep looking/testing.

jQuery(document).ready(function(){
    jQuery('textarea[rel*=atwhoMention]').bind("focus", function(event){
        jQuery.ajax({
            url: smf_scripturl + '?action=breezeajax;sa=usersmention',
            type: "GET",
            dataType: "json",
            success: function(result)
            {
                jQuery('textarea[rel*=atwhoMention]').atwho('@', {
                    search_key: "name",
                    tpl: "<li data-value='(${name}, ${id})'>${name} <small>${id}</small></li>",
                    data: result,
                    limit: 20,
                    callback: {
                        filter: function (query, data, search_key) {
                            return jQuery.map(data, function(item, i) {
                                return item[search_key].toLowerCase().indexOf(query) < 0 ? null : item
                            })
                        },
                    }
                });
            },
        });
    });
});

As a side note, my less than a year old Mac has decided it's display wants to fail so in the next few days, I have to send it to Apple for repairs. If I fall off the radar for a few days, it is because of this, but I'm hoping it'll hold out for a few more days so I can get some work done before sending it off.

@cheneyshreve01
Copy link
Collaborator Author

Hi @jywarren so with the query and without the underscore, e.g.

$.post('/tag/suggested/' + query, function(response) {}

still throws 404 error.

I think what is going on is related to the sequence that functions are called by the atwho object but not sure what exactly is going on with the search string
callbacks

@jywarren
Copy link
Member

jywarren commented Jul 20, 2018 via email

@jywarren
Copy link
Member

jywarren commented Jul 20, 2018 via email

@cheneyshreve01
Copy link
Collaborator Author

ok so with console.log(query) for statement without underscore, it does throw a 404 error first, but then it starts populating the letters..

screen shot 2018-07-20 at 12 19 38 pm

@jywarren
Copy link
Member

jywarren commented Jul 20, 2018 via email

@cheneyshreve01
Copy link
Collaborator Author

Hi @jywarren yes! The if condition gets rid of the 404 error so we can drop the underscore. Thanks for the tip!

@jywarren
Copy link
Member

jywarren commented Jul 21, 2018 via email

@cheneyshreve01
Copy link
Collaborator Author

Hi @jywarren sorry for any confusion, the [last commit] (d51276e) does have the changes we talked about with the conditional and no underscore.

@jywarren
Copy link
Member

Super I'll test this briefly today I hope then merge it! Thanks a million!

@jywarren jywarren mentioned this pull request Jul 21, 2018
Closed
@jywarren
Copy link
Member

OK, i used this a bit and ran into an encoding issue, and a few other things, so I made some small adjustments -- started including the emoji.json file in the page, rather than via a remote JS call. Then I wrestled with the display vs insertion formatters -- quite tough! But finally seemed to figure it out. The final changes are pretty small but I tested it out and it works /almost/ as well as we'd hoped.

I think the next step will be to deal with this encoding issue so we can display the emoji in the editor instead of just in the published comments: #2665

But for now this is ready to merge -- VERY EXCITING!!!

We have a few follow-ups, if you're interested (though, take a moment to celebrate, you've done a fantastic job here!)

  • we could move this JS code into its own /app/assets/javascripts/emoji.js file
  • we could generalize it so it could ALSO be used, or at least some parts re-used, for the rich editor, as you demoed above

In any case, THANK YOU!!!!! 🎉

Let's let this final version pass tests and I'll merge and publish it.

@ghost ghost assigned jywarren Jul 21, 2018
@ghost ghost added the in progress label Jul 21, 2018
@jywarren
Copy link
Member

Super, merging -- and done!

OK, if you'd like to do a few follow-up things, we should open a new issue and copy in the checklist from above (and maybe your animated gif!) to start a new thread separate from this very long one!

I hope you felt great about this and got a chance to build some useful skills! Thanks again!

@jywarren jywarren merged commit e95d893 into publiclab:master Jul 21, 2018
@ghost ghost removed the in progress label Jul 21, 2018
@jywarren
Copy link
Member

This is now live at PublicLab.org. Try it out! Say, on one of my posts:

https://publiclab.org/notes/warren/03-20-2018/collaboratively-document-an-event-with-a-mini-newspaper

@cheneyshreve01
Copy link
Collaborator Author

HI @jywarren awesome! I am definitely learning and grateful to have your support! I will copy/paste the checklist above and open a new issue. Thanks again!

@jywarren jywarren mentioned this pull request Jul 22, 2018
Closed
@jywarren jywarren mentioned this pull request Dec 12, 2018
Closed
rexagod
rexagod reviewed Jan 13, 2019
View reviewed changes
Copy link
Member

@rexagod rexagod left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't this be /api/srch/profiles?query=?

app/views/comments/_form.html.erb
at: "@",
callbacks: {
remoteFilter: function(query, callback) {
$.getJSON("/api/srch/profiles?srchString=" + query, {}, function(data) {
Copy link
Member

@rexagod rexagod Jan 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This returns {"error":"query is missing"}.

@jywarren
Copy link
Member

jywarren commented Jan 14, 2019 via email

@rexagod
Copy link
Member

rexagod commented Jan 15, 2019 •
edited
Loading

Okay! I'll send a PR with the corrections.

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019
@cheneyshreve01 @jywarren
[WIP] (please do not merge yet) Add autocompleting usernames with @ s…
b52da04 
…ymbol publiclab#2913 (publiclab#2925)

* initial setup commit

* [WIP] updated emoji to read from static file, upstream fetch & rebase

*  [WIP] added autocomplete username to comment window using atwho library

* [WIP] Add autocompleting usernames with @ symbol publiclab#2913 fixed typo

* [WIP] refactoring atwho calls

* finished refactoring using API call, adjusted indentations

* removed unnecessary example file

* [WIP] testing hashtag_config behavior, removed unnecessary files

* [WIP] refactored hash_config, removed unnecessary file, set limits for dropdowns

* [WIP] updated emoji code, rebased, removed unneccessary file

* [WIP] removed unneccsary atwho code from application_helper.rb

* updated hash call with conditional, removed query from emoji call, set limit for at callbacks to 20

* emoji fixes

* resolved publiclab#3120 ONLY_FULL_GROUP_BY issue w/ mysql 5.7+
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rexagod rexagod rexagod left review comments

@jywarren jywarren jywarren approved these changes

Assignees

@jywarren jywarren

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cheneyshreve01 @ViditChitkara @plotsbot @jywarren @rexagod @cheneyshreve