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.

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

@Mentions TODO #4640

Closed
11 of 12 tasks
lukebarnard1 opened this issue Jul 19, 2017 · 12 comments
Closed
11 of 12 tasks

@Mentions TODO #4640

lukebarnard1 opened this issue Jul 19, 2017 · 12 comments

Comments

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Jul 19, 2017

So far, mentions look like this:

2017-07-24-170223_497x252_scrot

There are a few things left to do on them before we release:

@ara4n
Copy link
Member

ara4n commented Jul 20, 2017

Some more thoughts:

@ara4n
Copy link
Member

ara4n commented Jul 20, 2017

  • Ability to create room mentions for arbitrary room aliases. Perhaps hitting tab after something that looks like a #user:domain.tld should turn it into a pill?

@t3chguy
Copy link
Member

t3chguy commented Jul 20, 2017

it seems like seequ means that rooms which were not known by the recipient were not made into pills

@ara4n
Copy link
Member

ara4n commented Jul 20, 2017

@uhoreg
Copy link
Member

uhoreg commented Jul 20, 2017

Consider removing disambiguation when rendering pills?

One possible way to do that is to wrap the disambiguation in a span with a special class (e.g. <a href="https://matrix.to/...">uhoreg<span class="mxid_disambiguation"> (@uhoreg:matrix.org)</span></a>), so that clients that understand the new mention syntax can just span.mxid_disambiguation { display: none; }

@turt2live
Copy link
Member

other options could be to make it an attribute on the pre-existing node, or to just throw it into the event content (although that might be a bit dirty?)

@lukebarnard1
Copy link
Contributor Author

lukebarnard1 commented Jul 21, 2017

To prevent abuse, we should be replacing the text content of the mentions with the actual display name or room alias of the resource linked anyway. It would be at that point where we would consider not inserting the disambiguation.

In terms of the implementation,

wrap the disambiguation in a span with a special class

would be done at the point we insert the disambiguation (after the message has been received) so perhaps we could just not insert it (and make that conditional on a user setting perhaps?) Not to mention allowing certain class names would be a bit weird during sanitisation.

make it an attribute on the pre-existing node, or to just throw it into the event content (although that might be a bit dirty?)

the receiving client has enough information to do the disambiguation itself so this feels redundant (although it's possible the client doesn't know about mentions (see next comment)). And yes, quite dirty 😛

@lukebarnard1
Copy link
Contributor Author

We'd keep it in the line format for compatibility with clients which don't understand mentions, but when we render the mention we could replace it with the current displayname for the user, and rely on hoverovers for disambiguation - a bit like we will be doing for flair)

SGTM, and we will be replacing the pill text anyway to prevent abuse.

@lukebarnard1
Copy link
Contributor Author

arbitrary room aliases.

Does it matter if there's no avatar? Currently there's no API for mapping #alias -> avatar

@ara4n
Copy link
Member

ara4n commented Jul 25, 2017

it's not a huge problem; bit of a shame though.

@resynth1943
Copy link
Contributor

I have another problem, but I'm not sure if it's covered (search didn't help):

If you mention people in the composer, then enter another key, the src attributes of the mention pills are re-set, making them load again.

I'm relatively sure browsers don't do an identity check on old and new src attributes, so any src set (regardless of identity) will end up re-requesting the image.

Would anyone like me to take a look at this?

@MadLittleMods
Copy link
Contributor

Closing as the last item in the issue description todo list is covered by #16981

@resynth1943 Please create a new separate issue if you're still seeing problem ⏩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants