Skip to content

Treat autocompletions as tokens with editing boundaries #6577

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.

Sign up for GitHub

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

Closed
brandonpayton wants to merge 16 commits into from
Closed

Treat autocompletions as tokens with editing boundaries #6577

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
b1a8d13
Wrap autocompletions as tokens
brandonpayton May 4, 2018
2db29d8
Handle editing tokens
brandonpayton Jun 7, 2018
6ecb723
Insert zero-width non-breaking space instead
brandonpayton Jun 10, 2018
8eef449
Update user completion style
brandonpayton Jun 10, 2018
6a4f2c9
Update the e2e test for zero-width space
brandonpayton Jun 10, 2018
20bb619
Style tweaks after discussion
brandonpayton Jun 12, 2018
137f9b6
Add a couple more why comments
brandonpayton Jun 13, 2018
9e8758b
Add 'element-closest' dep for `Element#matches`
brandonpayton Jun 13, 2018
5aa39e0
Insert token when spacing after exact match
brandonpayton Jun 26, 2018
62751a8
Make non-text completions readonly
brandonpayton Jun 27, 2018
6a94706
Clean up recent additions
brandonpayton Jun 28, 2018
5995871
Document different inserted token treatments
brandonpayton Jun 28, 2018
80e2234
Updates in response to code review
brandonpayton Jun 28, 2018
0c0f470
Fix missing input event for backspace
brandonpayton Jun 29, 2018
9380f6d
Fix incorrect insertion due to stale DOM range
brandonpayton Jul 2, 2018
23d2d93
Make autocompletions undoable
brandonpayton Jul 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add a couple more why comments
  • Loading branch information
@brandonpayton
brandonpayton committed Jun 29, 2018
commit 137f9b63663b3b948302ef6f2356e2a97fdd4d0b
7 changes: 5 additions & 2 deletions components/autocomplete/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,12 +234,15 @@ export class Autocomplete extends Component {
insertCompletion( range, replacement, completerName ) {
// Wrap completions so we can treat them as tokens with editing boundaries.
const tokenWrapper = document.createElement( 'span' );
Copy link
Member

@mtias mtias May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have a native tinymce API for this?

Copy link
Member

@ellatrix ellatrix May 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are TinyMCE APIs for this, but I'm guessing this component shouldn't depend on TinyMCE? Only the RichText component is aware of it. This is similar to writing flow which has tried to avoid using TinyMCE APIs. If we want we could pass the editor instance though.

tokenWrapper.innerHTML = renderToString( replacement );
Copy link
Member

@ellatrix ellatrix Jun 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the replacement is an element? Should we maybe only wrap in a span when it's a string?

Copy link
Member Author

@brandonpayton brandonpayton Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @iseulde, I think that's a great suggestion. If a completer wants to insert custom HTML, it could, and we wouldn't interfere by wrapping it in a token span. I am out of time today but plan to address this in the morning.

Copy link
Member Author

@brandonpayton brandonpayton Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'm not sure what to do here because, even if we don't wrap with a span, we probably want editing boundaries for any autocompletion. What do you think?

Should we add a class to the element if a completer returns one? What if a completer returns a fragment? This may have been the line of thinking I followed to wrap everything in a span, though I understand not all elements are valid children of a span.

Copy link
Member Author

@brandonpayton brandonpayton Jun 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After sleeping on this, my thinking is:

  • String completions should be wrapped as editable tokens. I believe this is the simple autocompletion experience we often expect.
  • Element completions should be marked as read-only since it is arbitrary content and may be awkward to edit properly.

What do you think, @mtias?

Copy link
Member

@mtias mtias Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like there could be two types of completers, but given lack of examples of the second kind, we could optimize for the cases we have now.

Copy link
Member Author

@brandonpayton brandonpayton Jun 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of the second kind where a completer is used to insert an abbreviation:
https://gist.github.com/brandonpayton/01b75c55ce4d329d534b1d3a68dc728f

second-completer-type-example

In this case, the completion does not include the trigger prefix, and its <abbr> element has a title attribute that cannot be edited via the RichText component. My take is that we should not wrap these in the same way but should mark them as readonly so the editor can remove but not edit them. Does that example help?

Copy link
Member

@mtias mtias Jun 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.


// Remember the completer in case the user wants to edit the token.
tokenWrapper.dataset.autocompleter = completerName;

// Add classes for general and completer-specific styling.
tokenWrapper.classList.add( 'autocomplete-token' );
tokenWrapper.classList.add( `autocomplete-token-${ completerName }` );

tokenWrapper.innerHTML = renderToString( replacement );

const existingTokenWrapper = this.getTokenWrapperNode( range.startContainer );
if ( existingTokenWrapper ) {
/**
Expand Down