-
Notifications
You must be signed in to change notification settings - Fork 8.6k
FEATURE: Validate Mentions in Rich Text Editor #32879
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good David, mostly minor comments, I tested it locally and it's working well 👍
app/assets/javascripts/discourse/app/static/prosemirror/extensions/mention.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/discourse/app/static/prosemirror/extensions/mention.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/discourse/app/static/prosemirror/extensions/mention.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/discourse/app/static/prosemirror/extensions/mention.js
Show resolved
Hide resolved
plugins({ pmState: { Plugin, PluginKey } }) { | ||
const plugin = new PluginKey("mention"); | ||
|
||
return new Plugin({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@renato I'm noticing a lot of these plugins/rules are ending up like an xmas tree of indentation, especially with plugins:
{
{
{
{
{
Maybe we can think about at some point storing the plugins in separate files, or definining them in some different way, even defining a class in the same file and hoisting it out of the main plugins()
method so it's a bit cleaner? Personally I am finding a lot of these prosemirror extension source files a bit of a slog to read through because of this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already doing so in some cases, but yeah we can always organize it in a clearer way. Splitting is not always welcome as it can add a lot of indirection, but for example the image-toolbar is a separate extension
The main issue with this extraction is that you'll notice that a lot of the dependency injection will still be necessary, because plugins and theme components can't access the ProseMirror packages directly and it's always a good idea to make our core extensions serve as examples for external customizations. This would be much simpler if (when?) we had a way to import these /static/ packages from TCs/plugins.
That said, feel free to extract or suggest anything that helps code clarity!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's a tricky problem, I guess it will improve over time, I do agree it's not always nicer to have things in a separate file, but there is a balance between that and everything inline in a massive function. Anyway nothing to do ATM, just something to be mindful of 👍
This change validates mentions for
users
andgroups
when using the rich text editor.When the mention is valid (ie. the user or group exists) it will visually show as a mention node, but when the user or group does not exist then it will appear as regular text in the editor.
There are 2 parts to this:
Existing drafts / toggling from Markdown to Rich Text
We automatically add mention nodes for all
@mentions
and then process any invalid mentions by removing the mention node and replacing it with text.When manually typing in Rich Text
We find the mention within the text and then if valid we replace it with a mention node.
Validation
The validation happens in a single request when loading a draft or switching from Markdown to Rich Text. The response provides a list of valid usernames and groups. It also provides additional context for groups and users that are not reachable (ie. unmentionable groups or suspended users etc) but we don't use this currently.
We then store the valid and invalid mentions to prevent unnecessary requests later.
As new mentions are typed then they will also be validated using the same request as above (if they are not already stored in our valid / invalid sets).
In the future we could take this further by invalidating unmentionable groups or suspended users, but at the moment this PR aims for feature parity in terms of UX with Markdown Editor.
How it looks
In this example we have a combination of valid and invalid mentions: