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

Mention #404

Merged
merged 2 commits into from
Jan 26, 2018
Merged

Mention #404

merged 2 commits into from
Jan 26, 2018

Conversation

sillych
Copy link
Contributor

@sillych sillych commented Jan 26, 2018

Support the new mention feature (Display Only)

2018-01-26-142618_832x268_scrot

will be

2018-01-26-142645_805x299_scrot

Copy link
Contributor

@dbrgn dbrgn left a comment

Choose a reason for hiding this comment

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

For my taste, the vertical padding of the mention box is a bit too large. Maybe we could reduce it a bit? Maybe 3px 5px?

src/filters.ts Outdated
let identity = possibleMention.substr(2, 8);
if (identity === '@@@@@@@@') {
text = text.replace(
new RegExp(possibleMention.replace(/([.*+?^=!:${}()|\[\]\/\\])/g, '\\$1'), 'g'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Whoa, I don't understand this RegEx. What does it do?

Couldn't we simply do text.replace('@[@@@@@@@@]', <span class="mention all">' + $translate.instant('messenger.ALL') + '</span>') first, and only then search for contact mentions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this saves one iteration, replace will be only called if one or more mentions (including the all mention) are found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check my new commit

it('mention - all', () => {
this.testPatterns([
['@[@@@@@@@@]', '<span class="mention all">messenger.ALL</span>'],
['@[@@@@@@@@] your base are belong to us', '<span class="mention all">messenger.ALL</span> your base are belong to us'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I approve of this test case!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@sillych
Copy link
Contributor Author

sillych commented Jan 26, 2018

padding changed to 2px 5px; 👾

@dbrgn
Copy link
Contributor

dbrgn commented Jan 26, 2018

When having mentions on two lines directly above each other, they partially overlap. But that's not too terrible, so it can still be fixed later :)

@dbrgn dbrgn merged commit 56efcaf into master Jan 26, 2018
@dbrgn dbrgn deleted the mention branch January 26, 2018 18:44
@rugk
Copy link
Contributor

rugk commented Jan 26, 2018

IMHO you should always still display the @ char at the beginning. Also in the app, if it is not shown there.
Just so users know that this is a mention and see they can do the same by entering that character. Just how every other char app does it, so…

@ovalseven8
Copy link
Contributor

When having mentions on two lines directly above each other, they partially overlap. But that's not too terrible, so it can still be fixed later :)

Better create an issue :D

@mimiks
Copy link

mimiks commented Jan 29, 2018

you should always still display the @ char at the beginning

The @ char looks pretty awkward. IMHO the grey frame is prominent enough.

@joelfischerr
Copy link
Contributor

But the @ character is pretty much the universal character used for mentioning someone. I agree with @rugk on showing the '@' char at the beginning of every mention.

@mimiks
Copy link

mimiks commented Jan 29, 2018

It might indeed be helpful to new users. But on the other hand, displaying the @ character at the beginning of a mention is redundant and doesn't look good.

@rugk
Copy link
Contributor

rugk commented Jan 29, 2018

It is done like that in every chat application I know. So it confuses me a bit that this is not shown, even if I am not a beginner. 😄
Also on GitHub it looks good this way, and seriously, no, there is no visual problem with an @ character. Or do you also don't like $ or # or what?

@mimiks
Copy link

mimiks commented Jan 29, 2018

It is done like that in every chat application I know

Don't think so. Check out Telegram for instance. And even if it was, we don't have to do it same way.

@joelfischerr
Copy link
Contributor

joelfischerr commented Jan 29, 2018

No, it's not redundant in my opinion as it clearly shows that someone is being mentioned. Just the highlighting of the text could lead to confusion as all names might be highlighted or it could be some formatting option. And as you can see in my reply above github shows the '@' character as well. I think it'd be better if it's done like everywhere else.

Don't think so. Check out Telegram for instance. And even if it was, we don't have to do it same way.

True Telegram doesn't show the '@'. But I still think that it's better to keep the '@' in.

@lgrahl
Copy link
Contributor

lgrahl commented Jan 29, 2018

@, # & co don't work for me here because real names contain spaces and that looks awkward (and FWIW would be awkward to parse, too).

@rugk
Copy link
Contributor

rugk commented Jan 29, 2018

In MyBB it also allows spaces... But you already did the parsing work, so there is nothing to worry, it is just a visual change (i.e., it can even be accomplished with a CSS pseudo-class ::before). (And I think the Threema IDs are used internally for the text, so it is not even a problem for the parser.)
It may look a bit surprising, but considering that you already have this grey background one clearly sees where the name starts and ends, i.e. it is just another character at the beginning, which is not a problem IMHO.

Maybe Threema can do an A/B test... 😄

@lgrahl
Copy link
Contributor

lgrahl commented Jan 29, 2018

Right, Threema IDs are used internally. The whole point of having an @ in front of it (for me) is that I can copy and paste that text and it does the same thing - mention a person. Which it wouldn't in this case, so it makes no sense.

@joelfischerr
Copy link
Contributor

It wouldn't preserve the mention when the text is copied back into Threema or when the text is copied elsewhere?
Because with the @in front of a name the meaning would be preserved in both cases. Without the @the meaning gets lost as soon as the text is shared.

@, # & co don't work for me here because real names contain spaces and that looks awkward (and FWIW would be awkward to parse, too).

I don't think it looks awkward. The highlighting makes the block @Max Musterman look like one single entity.

@rugk
Copy link
Contributor

rugk commented Jan 29, 2018

Because with the @in front of a name the meaning would be preserved in both cases.

No, because internally it is @ZU732H or so, with the Threema ID. Threema just shows the name instead of the ID.
So either it needs to parse the name, too, in @ statements (and here we get to the spaces problem), or you just cannot copy the element.

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

Successfully merging this pull request may close these issues.

8 participants