Forem: @ mention autocompletes in comment box Frontend

[benhalpern]

Feature Request or Task

As a user I want to be able to start typing with @ and have it deliver me a dropdown. This is expected behavior in this context.

This means that the box would have to become a content-editable div and the code should attach to each box. Making that part work could involve some yak shaving.

This should be written in Preact, and if think it makes sense to include a different library, let's discuss. We are critical of each dependency as we want to be efficient in this regard. It doesn't mean you definitely can't use a library, but let's just discuss the implications if you go that route.

The dropdown will use Elasticsearch, should search our user index

Definition of Done

This is done when all content boxes have a dropdown menu and the behavior works as expected on sites like GitHub and Twitter. Core team is here to help navigate the nuances of the issue.

[theoutlander]

I was about to make a request for autocompleting liquid tags and came across this issue. I can take a stab at it (need to get set up first). Following that, if there's consensus, I can look into autocompleting liquid tags.

[theoutlander]

Got set up after 3 hours of fighting everything. Need to get acquainted with a few things. I'm going to take a stab at a simpler issue (https://github.com/thepracticaldev/dev.to/issues/245) before I jump on this one.

[theoutlander]

@benhalpern is there an endpoint to retrieve user id's or would I need to create that as well?

Assuming I have to create it, there's the endpoint to serve aliases dynamically. I would end up caching the id's on the server-side since this will be a frequently used feature. This cache will also need to be updated as new users are added to the system (or removed). Ideally, we can do this as a micro-service unless you prefer that we stick to the current backend. Otherwise, there's a risk that we overload the backend. If we opt for a micro-service, would NodeJS / Serverless be an option?

As for the front-end, it will listen to a trigger event like '@' and query that endpoint to make automatic suggestions for aliases.

[theoutlander]

@benhalpern @pkfrank @jessleenyc @Zhao-Andy @maestromac I am ready to jump on this, but I need feature clarification as detailed in my previous comment.

Also, issues that are claimed should be assigned to that person. This is quite a problem at the moment when finding issues to work on.

[pkfrank]

Hey Nick, thanks for the feedback and nudge. I think that @benhalpern will clarify the endpoint question tomorrow, and we'll all continue to improve how we handle assignments to stay organized. We appreciate your help and contributions, and will be in touch soon!

[jessleenyc]

@benhalpern pinging you on this again.

@theoutlander -- we'd love to assign folks to issues but don't really know how to do it consistently! It seems like github only allows us to assign issues to core collaborators or the person who created the issue. See photo below. Is there a setting we're missing? @maestromac and I couldn't figure it out!

[benhalpern]

@theoutlander Users can be fetched via Algolia, and that is how we'd do that here. We already have an index set up for that. You can look at the way tags are fetched in the new editor for the general way way this would be done. Algolia is a low latency search built just for this sort of thing.

index.search(query, {
      hitsPerPage: 8,
  })

[theoutlander]

Oh, I see the problem. You will need to add the user as a collaborator to the repository. I think you can add them as readonly members and also restrict checkins into branches to admins only. I think that’s the only way to do it that I know of.

You can read more here: https://help.github.com/articles/repository-permission-levels-for-an-organization/

Thanks.

Sent from my iPhone

On Oct 15, 2018, at 3:21 PM, Jess Lee [e-mail]> wrote:

@benhalpernhttps://github.com/benhalpern pinging you on this again.

@theoutlanderhttps://github.com/theoutlander -- we'd love to assign folks to issues but don't really know how to do it consistently! It seems like github only allows us to assign issues to core collaborators or the person who created the issue. See photo below. Is there a setting we're missing? @maestromachttps://github.com/maestromac and I couldn't figure it out!

[https://camo.githubusercontent.com/e8446aae6401ba184df1565bb52492025c0361c7/68747470733a2f2f636c2e6c792f3965663563633965346537652f646f776e6c6f61642f496d616765253230323031382d31302d31352532306174253230362e32302e3434253230504d2e706e67]https://camo.githubusercontent.com/e8446aae6401ba184df1565bb52492025c0361c7/68747470733a2f2f636c2e6c792f3965663563633965346537652f646f776e6c6f61642f496d616765253230323031382d31302d31352532306174253230362e32302e3434253230504d2e706e67

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/thepracticaldev/dev.to/issues/354#issuecomment-430034586, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAtuHJoYUj6t-XN_9XWB2fXliUYsZzk7ks5ulQpvgaJpZM4V89Bq.

[theoutlander]

Thanks. That makes sense and it is the best option. I wasn’t sure if algolia had the user list. I’ll start digging into it this week.

Sent from my iPhone

On Oct 15, 2018, at 3:34 PM, Ben Halpern <[email protected][e-mail]> wrote:

@theoutlanderhttps://github.com/theoutlander Users can be fetched via Algolia, and that is how we'd do that here. We already have an index set up for that. You can look at the way tags are fetched in the new editor for the general way way this would be done. Algolia is a low latency search built just for this sort of thing.

index.search(query, {
hitsPerPage: 8,
})

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/thepracticaldev/dev.to/issues/354#issuecomment-430037507, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAtuHMmaGUvn_5-iUlQSjin7m3P7ZucPks5ulQ2KgaJpZM4V89Bq.

[rhymes]

@theoutlander @jessleenyc is there a way to create a team of "read only users" named collaborators? I guess with something like this https://help.github.com/articles/requesting-to-add-a-child-team/ - I'm not very knowledgeable in organization maintenance on GitHub

[theoutlander]

That is possible and I think that’s what we need. Perhaps users can be added to that list when they claim an issue. It’s also readonly so low risk. @jessleenyc you can experiment with me if you want.

Sent from my iPhone

On Oct 15, 2018, at 10:16 PM, rhymes <[email protected][e-mail]> wrote:

@theoutlanderhttps://github.com/theoutlander @jessleenychttps://github.com/jessleenyc is there a way to create a team of "read only users" named collaborators? I guess with something like this https://help.github.com/articles/requesting-to-add-a-child-team/ - I'm not very knowledgeable in organization maintenance on GitHub

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://github.com/thepracticaldev/dev.to/issues/354#issuecomment-430103649, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAtuHDvO_50bJIh5F_o6-9d9ZutmX3hVks5ulWubgaJpZM4V89Bq.

[jessleenyc]

@theoutlander sorry this has sat for a minute. I still need some time to play around with it. I think one of our concerns w/ having people 'assigned' is that it'll deter others from even looking into the issue. There are definitely times when people get busy and even though they've 'claimed' an issue, they're no longer working on it. I think the comments do a pretty good job of expressing if someone is working on an issue or not. What do you think?

[stale[bot]]

Thanks for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue in 7 days. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If this issue still requires attention, please respond with a comment. Happy Coding!

[pkfrank]

Un-stale.

[omenking]

I gave the v2 article form a look to see how it's being used with preact.
I am keeping to the existing convention and creating a new pack called commentForm
The trick here is articleForm only has to worry about one form on the page and so
document.getElementById('article-form') is an acceptable way to mount.

Since there can be multiple comment forms in articles#show will have to determine how to mount this for multiple elements or maybe it will just work with querySelectorAll iterating and mounting objects.

[omenking]

So here's my new pack, I think you can just iterate and it will mount, I will have to determine if scopes are respected later on.

import { h, render } from 'preact';
import CommentForm from '../comment-form/commentForm';

document.ready.then(function(){
  loadForms();
});

function loadForms(){
  console.log('loading forms')
  const elements = document.querySelectorAll('.comment-editor');
  console.log('el',elements)
  for (var i = 0; i < elements.length; i++){
    console.log('element',i,elements[i])
    render(
      <CommentForm />,
      elements[i],
      elements[i].firstElementChild,
    );
  }
}

Here I created myself a very simple component just to get a hello world going

import 'preact/devtools';
import { h, Component } from 'preact';

export default class CommentForm extends Component {
  constructor(props) {
    super(props);
    console.log('comment form')
  }
  componentDidMount() {
  }

  render() {
    return (
      <form
        className='commentForm__form'
      >
          testing 123
      </form>
    )
  }
}

Here we actually see the component rendering. I had gutted out all of comment form.

If this needs to be done in Preact then I think all of the comment form functionality has to be redone also in Preact and then we really need a ticket for that conversion and then this functantily is dependent on that new ticket.

I'm going to keep on working on this ticket.

[omenking]

In article form looks like we have some functionliaty already there that we should be able to pull from assuming these components were written to be resuable. Will give them a look

import BodyMarkdown from './elements/bodyMarkdown';
import BodyPreview from './elements/bodyPreview';

[nickytonline]

Just throwing in my 2 cents here.

A question about this feature. Are we planning on converting each comment to preact including the add new comment form and each can have the @mentions automplete or is the scope of this feature just the @mentions automplete component that gives the autocomplete value and then inserts it into the comment? If it's the latter, I would probably mount it off of maybe the comments container.

Also, since we want to avoid doing unnecessary work, consider only mounting this component when a comment is being edited. And when a comment was submitted, unmount the @mentions autocomplete component.

[omenking]

Autocomplete vs Preact Scope

The scope of this feature should be the autocomplete @mention but is dependent on converting to Preact. Preact should be its own ticket minus what is in this ticket.

What should be converted to Preact?

Should it be the entire comment with comment form or just the comment form?
I would suggest just comment form so we can piecemeal getting in the functionality that we want.
In an ideal world, I would say pull in the entire comments thread. Would converting the entire comments into preact save time down the road if done now? no I don't think so.

Baking the HTML

I have noticed that for the article form that they actually have all the html markup and then articleForm mounts to replace it. I'm guessing this was done as a quick and dirty way to get the article form to load really fast, maybe a quick and dirty way to get a fast loading page without having isomorphic code. eg. preact rendered server side.

[omenking]

I was able to bring over the toggle between markdown/preview functionality into the comment from the article form preact. I did notice in V2 of the article form editor it doesn't have markdown hotkeys. Maybe this is missing functionality in the new v2 article form.

[benhalpern]

What should be converted to Preact?

I think just the comment form here. The step of _converting to preact_ should be pretty minimal, as it will be taking the existing textarea and replacing it.

preact rendered server side.

This may be less minimal. If we can implement an effective server side rendering/automatic hydration, that would be ideal. I think that would be out of scope of this issue given that we could follow the established _dirty_ solution for now in order to get this functionality in.

The more rich client-side interaction we have, the more we need better approaches for handling how it is rendered and the better our underlying approach, the more rich client-side interaction we can have.

I'd be happy if this were implemented "dirty" _or_ if we prioritized improving the overall JavaScript code quality, or had folks work on both. But I don't think we should necessarily be paralyzed from both things due to waiting on the other. To this point I think that's sort of been the case.

[omenking]

@benhalpern I want to see this implemented so will avoid expanding the scope and be congruent with the existing code which so far has proved reusable. I will have more time to work on this ticket this upcoming Friday.

[omenking]

@nickytonline
How does this dataset get set in the packs/articleForm.jsx ? I can see dataset many places but don't know how they are getting applied to elements

Maybe @maestromac may know?

[omenking]

Think I found it. I guess dataset is a convention for preact to pull data attributes.

  <div class="articleform"
       id="article-form"
       data-article="<%= @article.to_json(only: %i[id title cached_tag_list published body_markdown main_image organization_id canonical_url], methods: %i[series all_series]) %>"
       data-organization="<%= @organization&.to_json(only: %i[name bg_color_hex text_color_hex], methods: [:profile_image_90]) %>">

[nickytonline]

It's a web standard thing. See HTMLElement.dataset on MDN.

[omenking]

The form is coming along, toggling between markdown and preview, populating it with data from dataset, proceeding to hook up submit button.

[omenking]

So sending this as payload, the old comments didn't send so much along and I could reduce this to just whats expected but trying to mimic articlesForms as best I can and honestly strong params will just filter anything not needed out.

[omenking]

I can submit comments but now comes the task of inserting a newly created comment.
Since the comments are not preacted I would have to insert HTML on the response.

So this is now as @nickytonline said early which is should all of the comments be converted to preact.

Now I am leaning to yes they should be.

[omenking]

Even if I wanted to preact all of the comments, there is this mess to deal with since there is all this caching for the nested comments which would have to preserve.

        <div class="comment-trees" id="comment-trees-container">
          <% if @article.comments_count > 0 %>
            <% Comment.tree_for(@article, @comments_to_show_count).each do |comment, sub_comments| %>
              <% cache ["comment_root_cached_tree", comment] do %>
                <%= tree_for(comment, sub_comments, @article) %>
              <% end %>
            <% end %>
          <% end %>
        </div>

[nickytonline]

Just on mobile, but I'll write something up here later tonight. Tldr though, I would keep it simple. First focus on the autocomplete component as it will be used in many places. Wherever it's used, I would load this component dynamically (dynamic import) probably on focus of an editable entity, E.g. comment. This way you can leave server-side rendered comments as is.

[omenking]

If we are moving forward with preact, let's just get it over with and preact-ify it.
If I have to rework the server side rendering I can.

[omenking]

Nick and I are going to get on Zoom at 10PM EST tonight and discuss @mention approach.
If anyone wants to jump in on the conversation here is the link:

https://zoom.us/j/121721734

@Zhao-Andy @benhalpern @rhymes @maestromac

[nickytonline]

Not Preact, just plain JS, but just a mini POC showing the idea of only attaching the comment autocomplete when necessary, https://codesandbox.io/s/5zn68rp3r4

[omenking]

Talk to Nick and I agree there is a more practical solution other than me preactifying all of the comments. Though just to show if you were curious I was going all-in and making comments as powerful as github comment but I will leave this for a future ticket and stay in the scope of @mention functionality

So the approach will be to listen on key press and then mount when needed a preact component, and this will still be inline for future preact functionality.

[benhalpern]

Hey, I'm happy to help out and get this moving along :) The last time this was discussed, we were swamped with other things. @omenking @nickytonline

[omenking]

So you can see in my last screenshot I was overhauling the comment form and I even was going as far as converting the posts to preact. The PR process was my main discouragement for completion.

It would be really nice if we stick with either v1 and v2 of the article forms. Since I see this task interconnected to multiple components.

[omenking]

@benhalpern if you can get on a Zoom call with me and talk about all the moving parts, it could help pave the way so when I PR this there's less friction in the review process.

[jessleenyc]

@omenking let's do it. want to send over some times/days that work for you this week?

[nickytonline]

I've got a lot going on at the moment, but happy to provide feedback/suggestions. I can join Zoom as well if you need me.

[omenking]

@jessleenyc

They only times I cannot do this week would be:
2PM Wednesday
11AM Thursday

I work from home so I can accommodate anytime of day around baby schedules
I'm going to suggest some evening times to be inclusive to Nick's schedule.

Here are suggested times:

Wednesday - 11am, 3pm, 7pm
Thursday - 3pm, 7pm, 9pm
Friday - 11pm 1pm, 3pm 7pm 9pm

[nickytonline]

I can do 3pm or 7pm on Thursday or 3pm Friday. Hopefully that works for everyone.

[jessleenyc]

Hey @omenking @nickytonline - just to confirm, what time zones are these suggestions in??

[jessleenyc]

Right now 3pm EST on Friday is looking best for us.

[nickytonline]

Eastern time for all of us. I believe Andrew is in Toronto.

[jessleenyc]

Great. Not sure if it's wise to share my zoom link on a public issue...unless there's a way to generate one-time video call invites that are scheduled? I can send you both calendar invites. Anyone else who wants to join, mention me here by Friday 8/16 @ 3pm EST and I can add you to the invite?

@nickytonline @omenking are the best emails to send the ones associated with your DEV account?

[nickytonline]

Yup, the associated email for me is the best one.

[jessleenyc]

Sent! @omenking lmk if you need it sent to a diff email and I can update the invite. Talk to you on Friday!

[omenking]

I'm in Toronto and I can accommodate either time chosen.
I have paid Zoom and I'm comfortable sharing a public Zoom link.

[jessleenyc]

@omenking you should have received a calendar invite with a zoom link already on it!

[omenking]

I was thinking why I didn't push my local branch and remembered as I attempted to push for today's meeting that the pre-commit hook made a mountain of work so I said forget it.

[mstruve]

Please note if you pickup this Issue that we are currently using Elasticsearch and that will have to factor into your implementation.

[mstruve]

@pkfrank: I'd at least like us to have a broad conversation about what it would take to "do this right." Ensuring Product + Engineering have a forum to discuss how this could happen.

From a backend perspective, this is very straight forward and we can implement it with an endpoint similar to tags. I do not have the depth to know the "correct way" here on the frontend. Let me throw this on my list, I might be able to bang out an endpoint pretty quickly for someone on the frontend to use.

@nickytonline I know you commented on this in the past. Are your comments still valid or would you suggest a different approach now given our frontend frameworks?

[nickytonline]

I would stick with what I mentioned in the past which is probably going with a 3rd party open source component called Downshift as its WAI-ARIA compliant. Also, our autocomplete component would be loaded dynamically, so it wouldn't add to the initial page load rendering time.

[mstruve]

Since we now have a backend endpoint for running username searches https://github.com/forem/forem/pull/11479 I am going to approve this as an EOY Priority and mark it for the frontend and our internal team.