Slack-style tag plugin (Resolves #1962) #2038
Conversation
matthew-carroll
changed the title
|
A couple change requests have been made:
|
…o when zero matches.
|
Video of latest two modifications Screen.Recording.2024-05-27.at.7.48.23.PM.mov |
|
Thank you @matthew-carroll ! I'll try it in a bit :) |
|
@matthew-carroll @josephrcox can we add to the specs: Search should not happen if @ is in the middle of the word (i.e search should happen if @ predecessor is a whitespace) Otherwise user typing a mail address would have the search popup opening after tappin @. |
| // FIXME: Use a transaction to bundle these changes so order doesn't matter. | ||
| if (removeComposingAttributionCommand != null) { | ||
| executor.executeCommand(removeComposingAttributionCommand); | ||
| print("Attributions immediately after removing composing attribution:"); |
There was a problem hiding this comment.
There are a few print statements that need to be changed to editorSlackTagsLoga
| // to reflect the deletions. | ||
| final composer = editContext.find<MutableDocumentComposer>(Editor.composerKey); | ||
|
|
||
| final baseBeforeDeletions = composer.selection!.base; |
There was a problem hiding this comment.
Selection should be checked if null before reading base.
Otherwise a scenario like editor.execute([ClearSelectionRequest)]) will throw an exception, letting the editor stuck.
| case SelectionChangeType.expandSelection: | ||
| throw AssertionError( | ||
| "A collapsed selection reported a SelectionChangeType for an expanded selection: ${selectionChangeEvent.changeType}\n${selectionChangeEvent.newSelection}"); | ||
| case SelectionChangeType.clearSelection: | ||
| throw AssertionError("Expected a collapsed selection but there was no selection."); |
There was a problem hiding this comment.
I would change these thrown errors with logs, at least for the beginning. For instance if you try the plugin on android with drag handles, moving the handle will throw here (SelectionChangeType.pushCaret). Editor is then stuck.
| if (composer.selection == null || !composer.selection!.isCollapsed) { | ||
| // We only tag when the selection is collapsed. Our selection is null or expanded. Return. | ||
| return null; | ||
| } | ||
| final selectionPosition = composer.selection!.extent; |
There was a problem hiding this comment.
same as above
| if (composer.selection == null || !composer.selection!.isCollapsed) { | |
| // We only tag when the selection is collapsed. Our selection is null or expanded. Return. | |
| return null; | |
| } | |
| final selectionPosition = composer.selection!.extent; | |
| final selection = composer.selection; | |
| if (selection == null) { | |
| return null; | |
| } | |
| if (!selection.isCollapsed) { | |
| // We only tag when the selection is collapsed or expanded. Return. | |
| return null; | |
| } | |
| final selectionPosition = selection.extent; |
| /// The key used to access the [SlackTagIndex] in an attached [Editor]. | ||
| static const slackTagIndexKey = "slackTagIndex"; | ||
|
|
||
| static const _trigger = "@"; |
There was a problem hiding this comment.
Please add a way to provide another trigger if the user wants to
| _requestHandlers = <EditRequestHandler>[ | ||
| (request) => | ||
| request is FillInComposingSlackTagRequest ? FillInComposingSlackTagCommand(_trigger, request.tag) : null, | ||
| (request) => request is CancelComposingSlackTagRequest // | ||
| ? const CancelComposingSlackTagCommand() | ||
| : null, | ||
| ]; |
There was a problem hiding this comment.
Can we think of letting package user provide custom handlers? Let's say in the example the popover enable the end user to pick not only a username, but also different kind of entity. The package user would use the plugin to get the composing value from the tagIndex, and send custom request to commit not only user tags.
| case SelectionChangeType.placeCaret: | ||
| case SelectionChangeType.pushCaret: | ||
| case SelectionChangeType.collapseSelection: | ||
| throw AssertionError( | ||
| "An expanded selection reported a SelectionChangeType for a collapsed selection: ${selectionChangeEvent.changeType}\n${selectionChangeEvent.newSelection}"); | ||
| case SelectionChangeType.clearSelection: | ||
| throw AssertionError("Expected a collapsed selection but there was no selection."); |
| /// An attribution for a slack tag that was previously being composed, but was then | ||
| /// abandoned (not cancelled) without committing it. | ||
| const slackTagUnboundAttribution = NamedAttribution("slack-tag-unbound"); |
|
@BazinC - As mentioned on our recent call, what I'm looking to get from you is a list of UX requirements to consider this feature done. Doing a per-line code review isn't relevant until this code actually does what's needed. Can you please provide the list of UX things this plugin needs to do, which it currently doesn't do? |
|
Hello @matthew-carroll, I understand this PR is not finished but I tried it and noticed bugs not related to any specific UX requirements so I found relevant to report them now, with repro steps and point the involved lines of code. Furthermore, I think the plugin could be more open to let Super Editor package users do more than it is possible in the current state. Requirements:
|
|
Note: We want to support triggers of both arbitrary content and arbitrary character count. We'll further discuss what team commitment means. |
This PR adds a new plugin which brings Slack-style tagging to Super Editor. Slack-style tagging is very similar to our existing "stable tag" plugin. However, Slack tagging appears to consider everything between the trigger character and the caret as the search term. Additionally, Slack tokenizes the search term and then matches names token by token.
The following videos demonstrate the plugin in action. The colors in these demos are optional and controllable. We use colors in the video to show exactly where the composing and stable attributions are applied.
Screen.Recording.2024-05-25.at.2.50.00.PM.mov
Screen.Recording.2024-05-25.at.2.50.54.PM.mov