Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/ckeditor5/998: Mentions MVP #2

Merged
merged 96 commits into from
Mar 27, 2019
Merged

T/ckeditor5/998: Mentions MVP #2

merged 96 commits into from
Mar 27, 2019

Conversation

jodator
Copy link
Contributor

@jodator jodator commented Mar 20, 2019

Feature: The mentions feature. Closes ckeditor/ckeditor5#998.

--

Edit: I've moved the styles to theme lark. The required PR is here: ckeditor/ckeditor5-theme-lark#221.

@jodator
Copy link
Contributor Author

jodator commented Mar 25, 2019

Since the PR is still open I've added the feature guide stub. I'll expand it tomorrow.

@jodator jodator requested review from oleq, Reinmar and mlewand and removed request for Reinmar March 26, 2019 12:15
@dkonopka
Copy link

dkonopka commented Mar 26, 2019

  1. We should keep balloon position "stick" to the line-height of text. Is it by default that 2-3px of top difference or just bug?

Screenshot 2019-03-26 at 13 32 56

  1. Are we going to have this following caret panel in MVP?
    dance-dance-dance

  2. We should add default color/background support for mention inside theme-lark, this reddish is IMHO a good choice. We need to remember about adding these variables to the guide.

:root {
    --ck-color-mention-background: hsla(341, 100%, 30%, 0.1);
    --ck-color-mention-text: hsl(341, 100%, 30%);
}
  1. Take a look here:
    4.1. I see some issues with balloon positioning
    4.2 Trying to mention someone without a space before @. Is it by design?

bugs

  1. One more thing, I'm not sure if "it's a bug or a feature" 😄
    hmhm

@jodator
Copy link
Contributor Author

jodator commented Mar 26, 2019

1. Trying to mention someone without a space before `@`. Is it by design?

yes - might be changed but this also covers:

  • @@@@
  • user@example.com

@Reinmar
Copy link
Member

Reinmar commented Mar 26, 2019

  1. Are we going to have this following caret panel in MVP?

I think we agreed it should stick to @ even in MVP. This is definitely a critical followup for me.

@jodator
Copy link
Contributor Author

jodator commented Mar 27, 2019

@Reinmar I see some follow-ups reported is this PR to be closed or something needs updating before merge?

@jodator
Copy link
Contributor Author

jodator commented Mar 27, 2019

We should keep balloon position "stick" to the line-height of text. Is it by default that 2-3px of

Will be fixed with #7.

Are we going to have this following caret panel in MVP?

in #7

We should add default color/background support for mention inside

I'll add it in this PR: ckeditor/ckeditor5-theme-lark#221.

One more thing, I'm not sure if "it's a bug or a feature" smile

Removing mention on typing - feature ;)

@Reinmar
Copy link
Member

Reinmar commented Mar 27, 2019

I confirm that it works 🎉

Will review the rest once it's on master.

@Reinmar Reinmar merged commit 3118814 into master Mar 27, 2019
@Reinmar Reinmar deleted the t/ckeditor5/998 branch March 27, 2019 16:02
@Reinmar
Copy link
Member

Reinmar commented Mar 27, 2019

@jodator, could you open tickets for all the remaining issues that were mentioned in this PR?

@robclancy
Copy link

Where is this on a roadmap for release? We need this soon and I want to know if I should just wait for a stable release or bring this in manually for now.

@mlewand
Copy link
Contributor

mlewand commented Apr 9, 2019

@robclancy we plan to do npm release later this week. So it's just around the corner. Either keep an eye on npm or just subscribe to releases of this package on GH and you'll get notified as soon as we publish it.

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

Successfully merging this pull request may close these issues.

6 participants