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

Postfixer should remove the whole mention when it's build from two+ text nodes #4622

Closed
Reinmar opened this issue Mar 27, 2019 · 8 comments · Fixed by ckeditor/ckeditor5-mention#17
Assignees
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Mar 27, 2019

Mar-27-2019 09-47-15

@jodator
Copy link
Contributor

jodator commented Mar 27, 2019

I was thinking about such cases and forgot to create a follow-up for this. I wonder how mention should behave when someone would try to style the mention?

Right now it allows to style part of a mention but shouldn't we disallow this? Enforce whole text node to be selected when applying attribute?

@Reinmar
Copy link
Member Author

Reinmar commented Mar 28, 2019

It's tricky... The most problematic part is that in the output you'll get a split span, so processing such content would be hard.

We've got two options:

  • prevent applying styling to part of a mention by either extending the style to the whole mention or not applying it,
  • remove the mention when part of it was styled.

The latter seems easier.

Do we have some examples how others approached this?

@jodator
Copy link
Contributor

jodator commented Mar 28, 2019

Quick check:

Slack breaks mention into "partial match" mention:

Selection_257

Github extends breaks bold:
foo @Rein**mar bar**: foo @Reinmar bar (different user is mentioned)

Gitlab - broken mention:
Selection_260

Twitter, Facebook - no styling no problem

@Reinmar
Copy link
Member Author

Reinmar commented Mar 29, 2019

I'd go with an easier solution for now. Which, IMO, may be to remove the mention. But if extending the style is easy to implement as well, then I'm fine with it too.

@oleq
Copy link
Member

oleq commented Mar 29, 2019

I agree, styling a part of the mention should remove that part from the mention. We can go with something more sophisticated in the future but for now, this will suffice.

@jodator
Copy link
Contributor

jodator commented Mar 29, 2019

should remove that part from the mention

To be precise - it should remove whole mentino as we do not allow partial mentions in other cases (ie typing).

@oleq
Copy link
Member

oleq commented Mar 29, 2019

Partial removal makes more sense IMO but I guess it's harder to implement, right?

@jodator
Copy link
Contributor

jodator commented Mar 30, 2019

Partial removal makes more sense IMO but I guess it's harder to implement, right?

We agreed that mention is full ie @jodator not @jodat so in that spirit we remove partial mentions.

@jodator jodator self-assigned this Apr 1, 2019
scofalik referenced this issue in ckeditor/ckeditor5-mention Apr 2, 2019
Fix: Mention post-fixer now correctly handles partial mentions on insert, remove and paste. Closes #12. Closes #8.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-mention Oct 9, 2019
@mlewand mlewand added this to the iteration 23 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:mention labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:mention type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants