-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Multiple triggers on mention component #6300
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
🦋 Changeset detectedLatest commit: 40d2f3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 57 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@tiptap/core
@tiptap/extension-blockquote
@tiptap/extension-bold
@tiptap/extension-bubble-menu
@tiptap/extension-bullet-list
@tiptap/extension-code
@tiptap/extension-code-block-lowlight
@tiptap/extension-code-block
@tiptap/extension-collaboration
@tiptap/extension-collaboration-caret
@tiptap/extension-color
@tiptap/extension-document
@tiptap/extension-floating-menu
@tiptap/extension-font-family
@tiptap/extension-hard-break
@tiptap/extension-heading
@tiptap/extension-highlight
@tiptap/extension-horizontal-rule
@tiptap/extension-image
@tiptap/extension-italic
@tiptap/extension-link
@tiptap/extension-list
@tiptap/extension-mention
@tiptap/extension-ordered-list
@tiptap/extension-paragraph
@tiptap/extension-strike
@tiptap/extension-subscript
@tiptap/extension-superscript
@tiptap/extension-table
@tiptap/extension-text
@tiptap/extension-text-align
@tiptap/extension-text-style
@tiptap/extension-typography
@tiptap/extension-underline
@tiptap/extension-youtube
@tiptap/extensions
@tiptap/html
@tiptap/pm
@tiptap/react
@tiptap/starter-kit
@tiptap/static-renderer
@tiptap/suggestion
@tiptap/vue-2
@tiptap/vue-3
commit: |
}) | ||
}) | ||
|
||
this.storage.getSuggestionFromChar = char => { |
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.
No problem to do something like this but it brings me back to an idea I had before. It feels weird to store internal functions in storage as I'd expect storage to contain data useful for the users.
In the future in another breaking change we could maybe change the API of the Extension.create
call to accept a callback function like this:
// we could maybe even have editor/other context related things as arguments
const MyNode = Node.create((editor) => {
function internalHelperFunction() {
}
const internalVar = "my-internal-var"
return {
// ... your extension config as usual
}
})
what are your thoughts?
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.
But no need to do anything right now
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.
I agree with this. Sometimes a helper function can help you reuse duplicated code.
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.
In general having an enclosed scope for an extension can be really helpful. We could make this change retroactive by allowing both a callback function & the old way of creating extensions.
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.
Created a PR for this:
#6301
Maybe we can merge this and use this instead of using storage?
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.
LGTM
It would be nice if we could have another test case that handles multiple suggestion triggers? |
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.
Pull Request Overview
This PR updates the Mention extension to support multiple suggestion triggers, allowing different suggestion configurations (e.g., '@' for people and '#' for places) and improves demos for both Vue and React. Key changes include:
- Introduction of a new utility function to generate suggestion options.
- Modifications to the Mention extension API and rendering functions to handle multiple triggers.
- Updates to demo examples and test spec placeholders for multi-trigger mentions.
Reviewed Changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
packages/extension-mention/src/utils/get-default-suggestion-attributes.ts | New utility to generate suggestion options based on provided overrides. |
packages/extension-mention/src/mention.ts | Updated Mention extension to support multiple triggers and deprecate renderLabel in favor of renderText/renderHTML. |
demos/src/Examples/MultiMention/Vue/suggestions.js | Added support for multiple trigger characters in the Vue demo. |
demos/src/Examples/MultiMention/Vue/index.vue | Updated Vue demo example to utilize multi-trigger mention functionality. |
demos/src/Examples/MultiMention/Vue/index.spec.js | Added test spec file placeholder. |
demos/src/Examples/MultiMention/Vue/MentionList.vue | Updated Vue mention list component. |
demos/src/Examples/MultiMention/React/suggestions.js | Updated React suggestions file to support multiple triggers. |
demos/src/Examples/MultiMention/React/index.spec.js | Added test spec file placeholder for React. |
demos/src/Examples/MultiMention/React/index.jsx | Updated React demo for multi-trigger mention. |
demos/src/Examples/MultiMention/React/MentionList.jsx | Updated React mention list component. |
demos/src/Commands/SetContent/React/index.spec.js | Adjusted test case to account for the new mentionSuggestionChar attribute. |
.changeset/wise-books-kiss.md | Changeset documentation for supporting multiple triggers in the Mention extension. |
Files not reviewed (2)
- demos/src/Examples/MultiMention/React/MentionList.scss: Language not supported
- demos/src/Examples/MultiMention/React/styles.scss: Language not supported
Comments suppressed due to low confidence (2)
demos/src/Examples/MultiMention/Vue/index.spec.js:6
- Add automated tests to validate the multi-trigger mention functionality in the Vue demo to ensure consistent behavior.
// TODO: Write tests
demos/src/Examples/MultiMention/React/index.spec.js:6
- Consider adding automated tests for the React multi-trigger mention demo to verify the proper functionality of the new configuration.
// TODO: Write tests
Ok, I'll add some tests on Monday |
@bdbch I just added some tests |
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.
LGTM! We'd just need to make sure the docs are up to date. But feel free to merge.
I'll update the |
Changes Overview
Support multiple triggers in mention component.
Example:
@
is used to mention people and#
is used to mention a place.Implementation Approach
Testing Done
Manually, with the demos. No automated tests. Let me know if you need automated tests.
Verification Steps
Additional Notes
The motivation of this PR is to enable a common workflow of AI assistant applications, where different resources (files, urls, users) are mentioned in the same prompt. It is also a requested feature in discussion #1768.
Checklist
Related Issues
#1768