theCrag / website

theCrag.com: Add your voice and help guide the development of the world's largest collaborative rock climbing & bouldering platform
https://www.thecrag.com/
112 stars 8 forks source link

@mentions in markdown for discussion, beta, ascents #831

Closed brendanheywood closed 9 years ago

brendanheywood commented 11 years ago

ETA Timeline: 6 months - 1 year #1253

Basically adopt the common convention that most other sites have implemented: @mentions

https://support.twitter.com/articles/14023-what-are-replies-and-mentions

https://github.com/blog/821

Anywhere that markdown is accepted we should accept stuff like @scd @cgome. Just like github these will become auto linked to that persons profile. I see this having a lot of value in the 'history' beta field where lots of names are mentioned. In some context will have extra side effects:

Just like twitters mentioned page, we'd have a page, or notification etc that tell people 'Hey you've been mentioned in 10 ascents', you can see those ascents and then the 'I ticked that too' function.

Some places that take names, like the ascent form, are quite ambiguous about accepting a username, or a non-username, and it's not obvious how to add multiple people. With this in place we could quite easily just merge the 'with' field with the 'comment' field.

Implementation things:

brendanheywood commented 11 years ago

Forgot we'd logged this already almost 2 years ago! #34

brendanheywood commented 11 years ago

Paydirt! I've looked at so many libraries for like a year and finally found what we need to support this on the front end:

Demo: http://ichord.github.io/At.js/

Code: https://github.com/ichord/At.js

scd commented 11 years ago

a good thing we waited

brendanheywood commented 10 years ago

Some thoughts about how we go about implementing, specifically with the 'with' ascents field, and equally to the 'FA' route field, but may apply elsewhere too:

The big question is migrating old data. The simplest is we just don't. I think the UX for new routes and ticks will be pretty solid and guide people into using mentions so not worried about that. I can think of too many situatons where we false positively match a 'with' with an unrelated account, so maybe we only migrate a string 'gtas' into '@gtas' when those two account are already linked?

scd commented 10 years ago

We allow people to change login names, which could cause issues. On the whole I don't think this is done to much, so we could either

Facebook are not even requiring the @ symbol to allow linking to accounts. I think we should just go with your suggestion with user name only.

If somebody types in an @, then I think we can do some predictive text. In particular I might not know your actual user name so I could just start typing your full name and the system will pick it up.

False positives is a nightmare for data migration. It is only really an issue where somebody has done this multiple times so changing manually would be a real hassle. What about writing a script which is run on a per user basis upon request. False positives will not be an issue, we could implement some trickier rules and we get strong user engagement.

brendanheywood commented 10 years ago

This isn't an issue at all. An @ mention is really only for input and output, internally it is always stored as an id and translated both ways. We hashed this out ages ago and it's in the 'implementation things' list at the top.

Also with the right UX the @ is almost an internal thing itself. Eg for the 'with' field as soon as you start typing you will get a dropdown of your linked accounts, and then maybe a more general search of all accounts. In the FA field we could do the same thing but start with the top 15 users who have also done FA's at that crag, or maybe karma as a fallback, and then a general search. Also in general markdown like a forum comment we can trigger mention check in javascript land after 4 characters have been typed but it only matched on your linked accounts. Facebook (and us) can do this because the # of friends is quite small and we can do this purely in JS without ajax.

So essentially the only times you'll be forced to use a @ is when you want to link to someone you aren't friends with. The majority of people will never really need it, and the power user editor types will figure out like the usually do anyway for things like the history markdown etc

Also to clarify my statement about searching, these would all match (in say 'with' field), and when it matched it gives a dropbox:

Simo - matched because I'm linked to you and I've type 4 characters ormore
@scd  - matched @ with a username, or partial username
@Sim - matched @ with a first and maybe also last name

If I select you it then writes @scd into the text field, which when saved gets parsed and translated to @9068185. In most places it won't actually be a textarea anymore but a html div with contenteditable turned on, and instead of @scd it will actually render a div with 'Simon Dale' and store the @9068185 itself in a hidden field. The 'Simon Dale' div could even be a bit fancy with a micro avatar or whatever else we desire.

scd commented 10 years ago

perfect

scd commented 9 years ago

Maybe we can get on to this issue soon. I want to make sure I understand all the scope of all the tasks involved.

API work

We need the api to handle text completion of user names (first name, last name & login) from

UI work

Integration of the at.js script using business rules for editing:

If we go to edit the field it should be in the form @somename . When reading existing data in an edit field. We should also probably pass it into the api for updating in the same format.

Storing

I am still not convinced that we should automatically map the @somename to an @1234. It solves the problem with changing user names (which happens rarely) but does potentially have transparency issues. I think this will be the first time we map input before we store it (other than to strip unsafe javascript).

At the moment I am assuming we store as @1234, but just noting my hesitancy.

Markup

When we see a @somebody or @12345 then we map to user name (in text mode) and marked up html in html mode. There is also a third mode which is form field mode, where we should map it to @somename

Facets

So far all of the above can be done without any db changes. If we have a Mentions table we can link accounts to various records and make for standard facet search coding. Following fields:

This would also make user notification easy, but I am happy to leave user notification to last.

brendanheywood commented 9 years ago

Woohoo!

API: yup, a single api point (already exists?) with an optional 'context' param which is an id of an area / photo / forum etc. The results will be grouped by those 3 types and in that order too. Also return an extra flag stating wether there are potentially more results, ie if you start typing 'benten' and it returns 10 results, and the flag saying that there are only 10 results, then typing 'bentend' won't make another network request it will just filter the existing date down further as it is instant. It will actually do this ahead of time with the data it already has before it makes subsequent network requests.

UI: Also annotations and profile 'about me' (added above). Also in some places, where we only want a list of people and not other text, we're better off using Select2 instead of at.js - in particular the ascent 'with' field. We can still use this to enter non linked text too for non members.

Storing: This is roundtrip, so when we display we map back to the username so I think that solves your transparency issue? The other alternative is when the mentions table is in place, if someone's username changes we know where to look to go search and replace them. It all comes down to when you want to do the processing, I'm fine with either way.

Markup: Yup - although your 'third mode' should be exactly the same as the first 'text mode'

Facets and linking: we also need to hash out how this meshed with existing linkages. eg we already have a link between who is in a forum discussion. So if you are mentioned in a comment you should be added to the forum, unless you've already been there and removed yourself explicitly.

So do we want to use this new mentions table as the primary data for 'fa links' and 'ascent with' links?

Also do we want to merge under the hood 'ascent with' and 'ascent comment mention' ie in almost all cases you'll say something like "@scd on lead and I shat myself" and you'll conceptually want it to mean the same as using the 'with' field. This is how FB and twitter work. Or we keep them separate which covers the much rarer case of things like "I finally nailed this after @scd showed it to me over a year ago". My thoughts are that as people get more comfortable with entering mentions into comments, also entering them into the 'with' field will seem like unnecessary duplication. If so the linkages from both are treated exactly the same, so if you are mention in 1 or the other or even both then that has the same effect semantically, eg potentially showing that ascent in the other persons feed, mentions alerts in your feed, what is shown when filtering in the ascent facets

scd commented 9 years ago

So do we want to use this new mentions table as the primary data for 'fa links' and 'ascent with' links?

Yes I think so.

Also do we want to merge under the hood 'ascent with' and 'ascent comment mention'

Not sure about this. I want to keep a separate field for 'with' because it prompts people to do it. It is an important piece of ascent information, so I think keeping it separate rather than incidental.

Let's implement without merging then see what falls out, and maybe merge later.

scd commented 9 years ago

FYI, I have tested At.js and it will do exactly what we want. Multiple completions (eg mentions, hashtags, route quotes, emojis, etc). It will also allow us to produce a dynamic list from the server using the autocomplete prefix already type in by the user.

I have started the work in completions.js. I think it will be fairly simple to get #tags working as well, and route quotes (when somebody starts typing a quote we could look up route names). I might give this a go, while I am doing the work.

I have decided to keep the logic simple for the mentions until we get it all working - if there is less than 2 characters typed then just get people you are following, otherwise get both people you are following and everybody with login prefix matching.

brendanheywood commented 9 years ago

very cool :)

Done any testing on mobile? We may need to disable this completely on some devices, so need a good runtime test for whether it's supported

scd commented 9 years ago

it does not work on my mobile, it calls the api end point but does not display any data. How do I detect and disable for incompatable devices?

Also got the hashtag completion working. This will be cool.

brendanheywood commented 9 years ago

It depends on what browser api's at.js and caret.js use internally. A quick look reveals dom input ranges at the very least:

http://caniuse.com/#feat=input-range http://stackoverflow.com/questions/15089962/feature-detect-support-for-dom-ranges-modernizr

however this should be supported on most devices? so probably a bunch of api's we need to check that exists. It is possibly that is could work but just has a bug, see this:

https://github.com/ichord/At.js/issues/155

scd commented 9 years ago

I have added a simple quote completion, which is also awesome. It's in repo. It currently only works in existing discussions for a crag.

brendanheywood commented 9 years ago

Docs for js dependencies for testing whether it's supported:

https://github.com/ichord/At.js/wiki/Techniques-being-used

A few little things:

image

image

scd commented 9 years ago

the dropbox often opens 'up' instead of 'down

This is controlled by the plugin. I don't plan to do anything about this.

the search should return following first, then others associated with the area / forum / photo, then randoms, eg '@simon' doesn't show you

Depending if we decide to include the search in the user's name @simon will not find me because my login is @scd. The way it works right now is that return a shortlist of all logins matching the prefix plus ALL the people you are following. This means you could get theoretically get me by typing @simon, but there are too many simon's in the list.

So we need to make a decision, do we try and tweak it so that @simon finds me?

Note that our API returns a shortlist, but the plugin decides what is shown.

it is searching inside usernames, 's' vs just a prefix search

See above business decision.

weird behaviour when only 1 or 2 chars, sometimes it searches and some times it doesn't. I think if you have entered a # or @ it should start searching right away, ie @ should simply list your friends, and # the top 10 hash tags. Only a ' based route search should wait for a couple chars before going to the network for suggestions

This is pretty much the behaviour I have defined. I am wondering if there is a bug. There is some caching and some code which stops the api calls once there are no matches on the short prefixes.

brendanheywood commented 9 years ago

This is controlled by the plugin. I don't plan to do anything about this.

It is controlled indirectly by how many results it shows. Currently this is 20, but it should be smaller, around 10. Showing it 'up' is really crap ux as it is almost always obscured by the floating header.

Here is how I think it should work: (not I'm putting a space after the @ so we don't invite randoms using github's mentions)

'@' by itself should show your top 10 friends, probably by alpha sorted. If less than 10, grab more from the area karma list and tack onto the end '@ s' should search for all your friends whose name or username starts with s. If there are less than 10, then also search the area for more. If still less than 10 then search the world. '@ si' exactly the same as above '@ sim' exactly the same as above

It should never display more than 10 results.

The match on username should always be a prefix only search. The match on display name I'd be happy with just prefix but perhaps it would also search on words, eg '@ dale' will match 'simon dale'. Certainly '@ mon' and '@ ale' should not need to match you.

scd commented 9 years ago

emoji completions are in repo

scd commented 9 years ago

I have fixed all the low hanging fruit for the x4 completion types based the discussion over the last coupled of days.

@brendanheywood please have look for further comments.

brendanheywood commented 9 years ago

mentions still feels a bit weird. '@ chris' doesn't find '@ cee - Chris L' who I do follow.

1) Also the sort is a bit off. If I type '@' then I get 10 friends - good 2) If I then type '@ e' I see '@ engramchris' and only him - mostly good 3) If I then type '@ en' then I would expect to still also see @ engramchris but don't anymore, I see a whole bunch of global names and Chris isn't one of them,

So two things:

scd commented 9 years ago

At the moment the MySQL search is just on login field. Please confirm that you are suggesting we also do a search for actual name. I think we can do this for people you are following but there are performance issues for the general population. I think we need to do a REGEX search for first or last name, but this does not use an index, so is only appropriate for friends.

Ideally we would want the global search to return people ordered by Karma, then I think we can dump the anit-feature of less than two chars. Otherwise we are getting 10 randoms out of 80k when somebody first types in @

brendanheywood commented 9 years ago

I'd be happy to skip the regex part and only search on the name as is - that's 80% of the win. Do we already have a case insensitive index for this?

Re the 10 randoms, this would only happen if they are a new user and not following anyone. But even then if they are in a forum for a crag like araps then the @ should show the top 10 for that discussion / crag, and only if that is less then 10 do a global search. I was also wondering if there is value in having another layer of 'country' between the crag search and the global search?

scd commented 9 years ago

our mysql is case insensitive including regexp

Have a look now as I have made some really sweat improvements (mostly what you suggested)

Country may be worthwhile. This concept comes up over and over again in other scenarios. I think we should probably make it a more formal part of our design like TLC. Not for now.

brendanheywood commented 9 years ago

Now that I've used this a bit, I agree and don't think I want global search either. In probably 90% cases I'd want to mention someone I know, and of the remainder it would be someone related to the crag. So I think leave that as is. Github is quite restricted in who is suggests, no accidental spamming.

Did some tweaking:

image

I am really happy with, now we just gotta get the auto linking in the markdown side done :)

scd commented 9 years ago

I have just done a large chunk of work here. Mentions are now linked into the following places:

It is stored as an ID in the database text fields.

I have written a script to back populate mentions in a few places. Although I have code for all of the above the only ones which really matter is route history, photos and ascents.

Mentions in photos means we can considate the photo fields to the photographer and climber text fields. We no longer need the id fields for climber and photographer. I have included script processing to convert them into mentions.

Facetted search implemented for routes by/scd and ascents with/scd.

TO DO:

brendanheywood commented 9 years ago

How do we want to treat the mentions in html? In almost all cases I think it's preferable to use the real persons name not their username. Usernames make more sense in github land where everyone is a coder. With us it seems that most people use their real name, but their usernames can be pretty random.

A mockup image

It makes the most sense to me if what they see in the autocomplete suggestion is pretty close to what they see when it's in html. In the FA and also when many people are mentioned in ascents or in an area history we will need them to work well in inline body text. So maybe something more like this:

image

brendanheywood commented 9 years ago

Some bugs:

scd commented 9 years ago

It is a bit fiddly integrating, I thought I caught most of the id/username edit conversions. Just let me know the form when you come across it. I presume the bug above is in your profile text.

Actually it will probably take us a while to sort out a lot of minor bugs, but I am really happy with how it has come out so far.

I agree that the HTML should convert into something like Simon Dale @scd, but I think the avatar in the middle of the sentence is a bit much.

brendanheywood commented 9 years ago

I think the regex's for all the kinda things need for the suffix part to be zero width look ahead assertions, so that multiple matching in a row don't match the overlap whitespace or comma between them:

image

I've fixed this but more of these probably in other places

brendanheywood commented 9 years ago

Also the auto complete inside the profile settings are a bit off:

image

also bad in the bulk update, but good in the forums

brendanheywood commented 9 years ago

I agree that the HTML should convert into something like Simon Dale @scd, but I think the avatar in the middle of the sentence is a bit much.

Ok just go with 'Simon Dale @scd' and I'll style it up

I think the root cause of the @ mention mismatch might be subtle differences in the regex's in Mentions.pm vs Markdown.pm I'm thinking to make maintenance easier if you make the regex parts into static vars that get reused so they are consistent + some liberal use of /x and whitespace to them easier to read

brendanheywood commented 9 years ago

Actually it will probably take us a while to sort out a lot of minor bugs, but I am really happy with how it has come out so far.

Yeah I am just kinda giddy that this has all come together so well, it's freaking cool :) great work, I'm sorta just a sleep deprived back seat driver ...

scd commented 9 years ago

Good call with putting the regex's into variables - it has sorted out some of the bugs you found, without having to debug.

Please pull from repo and have another test. BTW before you pull from repo you might want to read my email on my confusion in git sub modules.

brendanheywood commented 9 years ago

done

brendanheywood commented 9 years ago

I styled this up and played with it, and the more I play the more I think I don't want the @ username showing pretty well anywhere, not in with, inline in markdown etc. I was thinking I'd want it to reinforce the @ but I'm not so sure anymore, it is just starting to look like clutter, especially when you username is your name like mine. I think I just want to show the name, with the @ in the title, so done that and really prefer it, it's just normal simple linked text.

I previously thought it would help reinforce remembering usernames but that is irrelevant now, so it's only about know or reinforcing the @ concept which I think we can do in other better ways, like a dismissable dashboard / forums message popup like we've done in the past.

geez we must be pretty close to closing this?

brendanheywood commented 9 years ago
brendanheywood commented 9 years ago

And still something screwy with the edit-as-username stuff:

image

This is intermittent, the first time I went to edit that ascent it worked, then it half worked and one account got converted to a number and the others didn't. Then the next time none worked. If it was a busted regex as least it would be consistent so this feels like it's buggy internal state differences. Is it purely working of the markdown or is it looking at the existing mentions db records?

scd commented 9 years ago

I agree that just the persons name looks better. What if there is no name? It also means that text completion on @sim should pick up me even though my user name is @scd. We do this for people you are following, but we should make sure it is done as much as possible. I am not going to be able to do it for general usernames because there are too many.

The ids in the text forms will come up. It is just a mater of identifying them. I presume that the one above is the edit ascent form. There is no easy way to just fix this everywhere, rather throw in a mapping function for every form field where it is a problem.

I am not sure why you got the intermittent problem. I am pretty sure we don't cache ascent comments. Therefore I cannot explain it unless it is a browser reload issue (ie make a change then press the browser reload button - the browser preserves the changed text).

brendanheywood commented 9 years ago

Is no name possible? I thought was required, in any case just swap in the username instead. There is also one guy whose name is '.', I don't really want to add code for one user, but perhaps we could append the username if the name is < 3 chars.

I am not fussed about the autosuggestion for everyone as already discussed. If someone really want to link to someone they can cut and paste their username or follow them first etc. Retrospectively adding back lots of history is a different issue and I think covered pretty well in the proposed process in #558

The intermittent issue is definitely back end, this is inside a process so page caching doesn't apply. And it has been inconsistent within a single input field.

brendanheywood commented 9 years ago

I've implemented the no name case

scd commented 9 years ago

I introduced a bug in the regex when turning them into variables, which was the cause of the strange behavior. This has been fixed and is in repo. Please pull and test again.

brendanheywood commented 9 years ago

Hey just saw this in prod, is this just coincidence?

http://www.thecrag.com/photo/276422841

scd commented 9 years ago

Are you refering to the no name? None of the mention stuff is in prod yet.

brendanheywood commented 9 years ago

Prod is linked to the climber, but has no name.

Dev has the full name, and isn't linked.

This seems really odd, and back to front to the way it should be. Neither seems right.

This is a year old photo so I don't think the data has been updated, has there been some migration done on the dev data?

Prod: image

image

Dev: image

image

scd commented 9 years ago

Yes I have migrated linked climbers to mentions (I did mention it earlier in the discussion, but easily lost in the volume of updates).

Just for the avoidance of doubt, there is now only one way to link climbers in a photograph and that is through mentions.

In this instance we definitely want the avatar in climber field. I think there is a bug in the template for dev.

scd commented 9 years ago

It turns out there was an edge case bug with my mentions conversion. I did not think it was possible for there to be a name and a linked climber, so I ignored this case. In the photo record above the name 'Cath de Vaus' is the free text field (but no name in her account). Because of the edge case bug the linked climber did not come across to a mention. I have updated the migration so that the field is updated to Cath de Vaus @10686367.

I have rerun this on your dev @brendanheywood.

brendanheywood commented 9 years ago

Ok sounds like a few things had to align for this to all happen, so a lucky find.

but I still don't understand where the text 'cath de Vaus' even came from? In dev looking at the template data I can only find that string in two places and both are inside the 'streams' stuff which is data for sibling photos not this one. The 'climber' data doesn't mention that string at all:

image

As far as I can tell it is somehow magically pulling data from a completely different photo:

image

ie this one: http://dev.thecrag.com/photo/388659558

There is something voodoo going on here

Also on a completely unrelated note, there are places where the only valid input is a list of names, like the photographer and climber fields for photos, and the with field, and the discussion invite (even though the last two may go in future). In these places I think you probably shouldn't need to type the leading @ to autocomplete (like the forum invite). Additionally if you use free text with potentially interspersed @ mentions then in theory you could link to multiple people for the various fields.

I just tried this with a test photo and only the last mentioned name is linked, the rest is discarded:

http://dev.thecrag.com/photo/342242659

This is fine but we should be consistent one way or another, so for the photos we should only access a single name, and as soon as you start typing you get a dropdown of matching name. If nothing matched whatever you typed will still get added as free text. It probably does make sense that there could be multiple climbers in a photo, and even multiple photographers, but happy to stick with 1-1 links for photos.

The 'with' field I think if we keep it long term should behave the same way, but allow multiple people separated by commas. Again if stuff matches it auto completes, if not it's added as free text.

The 'forum invite' while already functioning and we've talked about removing, however I think is worth keeping, and also updating to be the same, except free text doesn't make sense here. The current implementation isn't great and in particular doesn't work with the usual keyboard events so makes it pretty hard to use.

So for these three use cases I think we use Select2 instead of at.js for just these fields which handles all cases of these perfectly. The dropdown we'd style to match at.js. There are probably other user list input fields I haven't thought of? If you are cool with this I'll start tinkering or I can point you in the right direction.

scd commented 9 years ago

'cath de Vaus' is not in the climber record, it is only in the photo record. Up until the recent change there were two fields for climber in the photo record 1) for linked climber and 2) for free text name. Both of these were filled in for the photo record, but the text one was discarded in prod. Since the migration the linked climber was discarded. Hence the difference. So there are a number of overlapping changes which may have confused things. It makes sense to me.

I am just thinking out loud, there maybe something we should test in the streams for new photos.

Yes, I dump all but the first (maybe last) mention in photos for the template display. This is just temporary because the template expects a single hash record not an array. We should note this on the TODO list.

I think we can commit to having Photographer as a single record, but we should allow mulitple climbers in a photograph.

I cannot remember how Select2 works, but will it allow for multiple and for plain text as well. The photo climber and ascent with fields both need this.

scd commented 9 years ago

Can we now close this, as the remaining issues are being manages separately. @brendanheywood please reopen if you disagree.