Skip to content
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

Autocomplete links to user posts #2896

Merged
merged 50 commits into from Nov 3, 2017

Conversation

@EphoxJames
Copy link
Contributor

commented Oct 6, 2017

Description

This implements autocompletion of user names as in #2793 . Currently it is only implemented on the paragraph block though that could be extended.
To try it out:

  1. Press '@' anywhere in the paragraph where there is a preceding space or the text is interrupted somehow.
  2. A menu listing all the users will be shown
  3. Start typing a user name to filter the list and user the UP/DOWN arrow keys to change the selected item.
  4. Press enter or click an item in the list to select it
  5. A link to the user posts will be inserted replacing the @query

How Has This Been Tested?

This has been tested with automated tests and manually.

Screenshots (jpeg or gifs if applicable):

After pressing '@':
user-autocomplete-1
After typing 'jam' to filter:
user-autocomplete-2
After pressing enter:
user-autocomplete-3

Types of changes

This is mostly a new feature built on top of the existing block-autocomplete and autocomplete features. I had to heavily modify the autocomplete feature to make it generic enough to work for user autocompletes. I also had to modify the popover code slightly to allow it to position over a node range.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@EphoxJames EphoxJames requested a review from youknowriad Oct 6, 2017

@youknowriad youknowriad requested a review from aduth Oct 6, 2017

onReplace={ onReplace }
placeholder={ placeholder || __( 'New Paragraph' ) }
<UserAutocomplete key="editable">
<BlockAutocomplete onReplace={ onReplace }>

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 6, 2017

Contributor

I haven't looked in details yet, but I'm wondering if we should create a separate Component for this or enhance the BlockAutocomplete component instead. I guess will have tags autocomplete too, so we may end-up with several levels of autocomplete.

Trying to think about what the API should look like, maybe I see something like this:

const blockPattern = {
  triggerOn: ( content ) => content.length === '', // when to trigger the autocomplete could use regex for instance
  getSuggestions: ( content ) => fetch( '/users' ), // A function triggers on each content change if the the triggerOn is true, it returns the list of suggestions (or a promise of list of suggestions)
  renderSuggestion: ( suggestion ) => <Button>{ suggestion.tittle }</Button> // Renders a unique suggestion
  onSelect: ( suggestion ) => // What happens when you select a suggestion
}

const patterns =[ blockPattern, userPattern, tagsPattern ];

<Autocomplete patterns={ patterns } ><Editable /></Autocomplete>

And the patterns could be declared in separate files for reusability.

Thoughts @aduth

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 9, 2017

Author Contributor

The API you suggest is insufficient. If you want it to work for triggers like '@' anywhere in the content then you also need to know the location of the cursor otherwise you will popup dialogs for items that match the trigger but you are nowhere near.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 9, 2017

Author Contributor

Also for things like the @user popups you only want them to happen outside of links because a link inside a link is invalid HTML so you have to work with the actual HTML not just the text.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 9, 2017

Author Contributor

Anyway, I am working on making it work like you suggest though I won't have it finished today.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 9, 2017

Contributor

Yes, I'm aware the API is probably not feature-proof but just wanted to show an idea on how we can build this generically. (which would ease extending these for plugins as well).

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 10, 2017

Author Contributor

I have updated the design of Autocomplete to take in multiple "completers".

@EphoxJames EphoxJames self-assigned this Oct 10, 2017

@EphoxJames

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2017

I think I have found a workable way of updating the tests so I started on that today. Still about a dozen tests to go.

@EphoxJames

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2017

@youknowriad If you have another chance please look over the changes and advise if you think it needs any more work or if you're happy.

@youknowriad
Copy link
Contributor

left a comment

Overall, this is going to the right direction IMO. What are your thoughts about the simplification I'm proposing

className: 'blocks-block-autocomplete',
triggerPrefix: '/',
getOptions,
allowContext,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 13, 2017

Contributor

Can't understand the difference between triggerPrefix and allowContext, could these be merged into one function?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 15, 2017

Author Contributor

TLDR: not without making it an annoying API to use.

triggerPrefix is the string that preceeds the query. For example "@" in the case of the user mentions, "/" in the case of block selection and "#" in the case of hash-tags (which aren't implemented yet). In theory you could just search back to the last space or boundary and pass the whole range to the completer definition to check but that that means that ALL completers would have to convert the range into text and check the leading substring. This puts more work on the implementer of the completer which is exactly where we don't want it.

allowContext was added to solve the problem that sometimes the completer has to know what preceeds it and follows it. The majority of the time this doesn't matter but the original implementation of the autocomplete block would require that there was nothing but whitespace before and afterwards (which makes sense because it replaces the whole block). This field is completely optional and most completers won't use it.

mention.textContent = '@' + user.name;
range.insertNode( mention );
range.setStartAfter( mention );
range.deleteContents();

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 13, 2017

Contributor

I wonder if the userAutocompleter should be aware of what to do with the selected value. This forces us to add a range property here where we could just execute a TinyMCE command if we were to have an instance of it.

Same could be said above for the blocks autocompleter, assuming there's only one action to be done when selecting a block: replacing.

What if the onSelect is always provided by the calling side:

<Autocomplete key="editable" completers={ [
	blockAutocompleter( block => onReplace( block ) ),
	userAutocompleter( user => this.editor.insertContent( linkFromUser( user ) ) ),
] }>

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 13, 2017

Contributor

I guess the difficulty here is that we need to remove what we already typed. Maybe we should do this consistently before calling the onSelect

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 16, 2017

Author Contributor

The advantage of Range is that it is an editor agnostic API and in the past I have been told I can't assume that components will always work with TinyMCE. I specifically did not want the autocomplete code to make any assumptions about what would be done with the range.
Note that TinyMCE doesn't really care about us changing the content out from under it and it would be breaking the abstraction that Editable is suppose to provide.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Also one of the advantages of using range is that TinyMCE knows how to work with ranges.
For example if TinyMCE was available as this.editor then you could do:

(user, range) => {
  this.editor.selection.setRng( range );
  this.editor.insertContent( linkFromUser( user ) );
}

That would select the already typed content and immediately replace it with the inserted content.

<Autocomplete key="editable" completers={ [
blockAutocompleter( { onReplace } ),
userAutocompleter(),
] }>

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 13, 2017

Contributor

Nice 👍

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 13, 2017

Contributor

Thinking the "patterns" we have right now in Editable could be written like this cc @iseulde

@@ -3,11 +3,12 @@
*/

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 13, 2017

Contributor

I'm not comfortable reviewing the changes in this file, I feel like it could be simplified if we do the generic onSelect callback. I believe @aduth created this file originally and would love if you have time to look at these changes (maybe after the change proposed here #2896 (comment))

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

I am happy to explain what I've done line by line if it would help?

@EphoxJames

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2017

@youknowriad I have tried to simplify onSelect further by allowing the provider to simply return the react html that they want to replace the range with - it will render it to html and insert it into the page and remove the autocomplete trigger and query. That way authors of autocompleters don't have to even know what a range is but it is available if they know and want to do something more complex.

}
}

reset() {
this.setState( this.constructor.getInitialState() );
}

onBlur( event ) {

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Explanation:
onBlur was removed because the aim of hiding the popup when clicking away could be better handled by using the onClickOutside function of Popover.

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

Problem here is if the user tabs away from the block while the autocomplete is still open, it remains open.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Well the original (as in the version in master) was hiding the Popover when you clicked on it meaning that options could only be selected by pressing enter. I will try to find a way to avoid both problems though.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

I ended up adding a handler for the tab key to close the menu.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

I realized that the left and right arrow keys had the same problem so they also close the menu now.

This comment has been minimized.

Copy link
@aduth

aduth Oct 25, 2017

Member

Well the original (as in the version in master) was hiding the Popover when you clicked on it meaning that options could only be selected by pressing enter.

Ah yeah, seeing this now on master. I can't say for certain that detecting specific keys as an implied blur is sufficient though. Do these differ at all by browser, user preferences, OS? Might be missing some like Escape.

Actually, in the case of Escape it may be working correctly, but only because Popover stops propagation when handling this itself. Quite fragile.

Seems like the problem is that since the popover is rendered elsewhere in the DOM, this.node.contains will return false when clicking on a button within the popoover. But since our component here is the one responsible for rendering the list, maybe a solution is to assign a ref for the ul and check whether the related target is also (or instead) in this list.

Might be a bug worth addressing in a separate pull request.

const container = event.target;
const cursor = this.getCursor( container );
// look for the trigger prefix and search query just before the cursor location
const match = ( function() {

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Explanation:
match() does the following...

  1. Search backwards from the cursor looking for the first space or element boundary (ie text highlighted bold)
  2. Then matches the trigger prefix of the completers to the text immediately after the space/element boundary.

In the process it will filter the list of completers by 2 other critera:

  1. All text nodes included in the match must be considered acceptable by the completer - this helps us check that we're not trying to create a link inside another link.
  2. The range before and after the match must be acceptable to the completer - this helps us ensure that we're not going to replace a block that actually has content in it.

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

Should this just be a separate function? getMatch ? The IIFE might not be an obvious pattern for the uninitiated reader.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

I extracted it out as findMatch

*/
export function userAutocompleter() {
const getOptions = () => {
return ( new wp.api.collections.Users() ).fetch().then( ( users ) => {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

We might consider the implications for users who don't have permissions to list other users on a site. Currently this doesn't appear to break anything, but will only ever return administrator users if issued by a non-administrator. Probably fine, with the other option being that we don't enable the autocompleter for these accounts.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

If you know of a better source for the information I will happily switch to it.

return {
value: user,
label: [
<img key="avatar" alt="" src={ user.avatar_urls[ 24 ] } />,

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

We should assign a meaningful alt here, maybe { __( 'User avatar' ) }

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

The problem with that is that screen readers will (for each item) read out "User avatar" before telling them the actually useful information of the user-name. Unless we can give an actually useful description it's probably more usable if they are just treated as decoration.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

@afercia do you think we should put descriptive text on the avatar in the users menu or treat it as decorative?

This comment has been minimized.

Copy link
@afercia

afercia Oct 20, 2017

Contributor

@EphoxJames empty alt are fine in this case. The relevant information is already in plain text close to the image, so these images are decorative.
Although not exactly the same thing, we've faced a similar case in the WP credits page. See https://core.trac.wordpress.org/changeset/36406 and https://core.trac.wordpress.org/ticket/34953.

}
}

reset() {
this.setState( this.constructor.getInitialState() );
}

onBlur( event ) {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

Problem here is if the user tabs away from the block while the autocomplete is still open, it remains open.

}

toggleKeyEvents( isListening ) {
// This exists because we must capture ENTER key presses before Editable.

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

If enter works correctly in master with the block autocomplete, why do we suddenly need this now?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

The enter key handler in master relied on overriding the keyhandler of the Editable. I'm not certain why it was able to get the event before TinyMCE when a onKeyDownCapture handler on the surrounding component was not - I guess the react simulated events don't mix with native events perfectly. When I was initially working on nesting these autocomplete handlers that approach was unworkable though now it is only one component I suppose we could return to it. That said I think that overriding the event handler on a child block is very weird and I much prefer this approach.

// Some completers must do asynchronous requests to get their options so
// we calculate that here.
completers.forEach( ( { getOptions }, idx ) => {
getOptions().then( ( options ) => {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

This behavior means we're issuing one request to the users endpoint for every paragraph block in a post. I'm not sure we need to be so proactive about querying this either? In some cases won't we need fresh data in response to a user search?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Good point - I will change it to populate the cache on the first non-empty search of a lookup.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

Now it loads options on first use.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

I have tweaked it so it also makes the getOptions call each time the autocompleter is initially triggered, so the information will be fresh, though query characters that narrow down the search will not cause a new getOptions call.

text.length - 1 :
Math.min( text.length - 1, fromIndex );
for ( let i = fromI; i >= 0; i-- ) {
if ( /\s/.test( text.charAt( i ) ) ) {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

This function is called on every keypress in a paragraph block, which is definitely not ideal, and if we're to keep it, should be subject to optimizing for performance. The RegExp test is not the fastest:

https://jsperf.com/character-whitespace

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

Maybe this could serve better than our own iterating implementation:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/search

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Well I suppose I could use a regex like:

/\s\S*$/.exec(text)

Still I have no idea if that is better than a loop searching from the end without benchmarking it and it would probably be highly dependent on the length of the text.

I should point out that:

' \t\n\r\v'.indexOf( testChar ) !== -1

is NOT equivalent for example it does not match nonbreaking space.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

I did some benchmarking and even on a bad case the existing code can run 655,000 times per second. I tried the regex that I mention above and of course which is better depends on the length of the string, the regex works better for short strings but once the string becomes long the search from the end wins. In short I don't think there is a good reason to replace this function.

const container = event.target;
const cursor = this.getCursor( container );
// look for the trigger prefix and search query just before the cursor location
const match = ( function() {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

Should this just be a separate function? getMatch ? The IIFE might not be an obvious pattern for the uninitiated reader.

}
}
// exit early if nothing can handle it
if ( text.substr( pos + 1 ).length === 0 ) {

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

Another optimization: We probably don't need to do a full substr if we're just testing if there's more text left, instead looking at the immediate next character:

if ( text[ pos + 1 ] === undefined ) {

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

Or:

if ( text.length <= pos + 1 )
>
<ul
role="menu"
className="components-autocomplete__results"
>
{ filteredOptions.map( ( option, index ) => (
<li
key={ option.value }
key={ option.key.toString() }

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

What's the case where the toString becomes necessary?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

According to react docs key should be a String - I wanted to use a number so I converted it to a string.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

From this page:
https://reactjs.org/docs/lists-and-keys.html
"A “key” is a special string attribute you need to include when creating lists of elements. "

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 18, 2017

Author Contributor

Anyway I have now changed this so the key is the combined indexes of the completer and the option separated by underscore so the toString is getting removed.

position="top right"
className={ classes }
range={ range }

This comment has been minimized.

Copy link
@aduth

aduth Oct 17, 2017

Member

I'm going to have to take a closer look at the needs here, but at a glance it doesn't appear range is intuitively a concern that Popover should be handling.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

I needed some way of positioning the Popover above a selection. Range seemed the best way to pass a selection.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Oct 17, 2017

Author Contributor

There is probably a better name for it though - range is rather generic.

@EphoxJames

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

@afercia I have tested the changes with NVDA and they seem to work though the menu items only seem to be read when I moved through them with the arrow keys. I also tried JAWS professional but I couldn't figure out how to edit anything (I think I was stuck in navigation mode) so I couldn't test if it was working. Do you think it is ready to merge?

@aduth @youknowriad Any further issues I need to address before merging? Do you disagree with my resolution of any of the existing issues? If not can someone approve the merge?

focusOnOpen={ false }
onClose={ () => this.reset() }

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

We should avoid creating a new function reference each time the component renders, since otherwise React will be forced to reconcile the change; this can be simply onClose={ this.reset }

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

Done.

if ( isLeft ) {
rect = rects[ 0 ];
} else if ( isRight ) {
rect = rects[ rects.length - 1 ];

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Reading through the documentation for Element#getClientRects, it's not clear to me what we're achieving by referencing the start/end of the returned collection?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

For the case of auto-completion it will do nothing because all the text will be on one line however if this was used on something that could be line wrapped then positioning over the first rect for left alignment and the last rect for right alignment seems to be sensible to me.

blockAutocompleter( { onReplace } ),
userAutocompleter(),
] }>
{ ( isExpanded, listBoxId, activeId ) => (

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Do we expect to need to enhance this with more options, and/or is the ordering important here? Wondering if we'd be better off specifying this as an object from which we can pick, so instead:

{ ( { isExpanded, listBoxId, activeId } ) => (

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 1, 2017

Author Contributor

It's unlikely that we'll need more options but I don't see a downside so sure.

className="components-autocomplete"
>
{ cloneElement( Children.only( children ), {
onInput: this.search,

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Since we added Editable "event proxying" specifically to allow for this, and given some of the challenges we've encountered with the event proxying, I'm wondering if now that we're no longer overriding these props that we drop the Editable logic:

proxyPropHandler( name ) {
return ( event ) => {
// TODO: Reconcile with `onFocus` instance handler which does not
// pass the event object. Otherwise we have double focus handling
// and editor instance being stored into state.
if ( name === 'Focus' ) {
return;
}
// Allow props an opportunity to handle the event, before default
// Editable behavior takes effect. Should the event be handled by a
// prop, it should `stopImmediatePropagation` on the event to stop
// continued event handling.
if ( 'function' === typeof this.props[ 'on' + name ] ) {
this.props[ 'on' + name ]( event );
}
};
}

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

Fine with me but should probably be in another PR.

// this method is separate so it can be overrided in tests
getCursor( container ) {
const selection = window.getSelection();
if ( selection.isCollapsed && container.contains( selection.anchorNode ) ) {

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Is there any chance we'd get to this point and container.contains( selection.anchorNode ) be false? Seems it could be fair to assume input event would only fire if selection is within the container? Trying to see if we can drop some logic which is run on every keystroke in Editable.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

Ok, I have moved it into an assertion so it is only run during development.

const range = this.createRange( startTextNode, pos + 1, endTextNode, endIndex );
const before = this.createRange( container, 0, startTextNode, pos + 1 );
const after = this.createRange( endTextNode, endIndex, container, container.childNodes.length );
if ( ! allowContext( before, range, after ) ) {

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Where are we using range?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

I couldn't think of a use case so I have removed it.

const before = this.createRange( container, 0, startTextNode, pos + 1 );
const after = this.createRange( endTextNode, endIndex, container, container.childNodes.length );
if ( ! allowContext( before, range, after ) ) {
return false;

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Seems like this could be simplified to return allowContext( before, range, after ); (or cast to boolean via !! is we might expect non-boolean return types)

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

Done - was a vestige of the original where there was only one completer and they were nested rather than a list.

const match = this.findMatch( container, cursor, completers, wasOpen );
const { open, query, range } = match || {};
// create a regular expression to filter the options
const search = open ? new RegExp( escapeStringRegexp( query ), 'i' ) : /./;

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Minor and unrelated to these changes: I discovered that Lodash has a utility for this built-in, so we could probably drop the extra dependency:

https://lodash.com/docs/4.17.4#escapeRegExp

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

Ok I will switch over to the lodash version.

This comment has been minimized.

Copy link
@aduth

aduth Nov 3, 2017

Member

This is the only place we were using escape-string-regexp so we should remove the dependency as well.

const [ yAxis, xAxis ] = this.getPositions();
const isTop = 'top' === yAxis;
const isLeft = 'left' === xAxis;
const isRight = 'right' === xAxis;

let rect = anchor.parentNode.getBoundingClientRect();

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Seems wasteful to call Element#getBoundingClientRect only to have chance of overriding if range is specified. This looks like something that could go into the else of the below condition:

let rect;
if ( range ) {
	// ...
} else {
	rect = anchor.parentNode.getBoundingClientRect();
}

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

Done.

const [ yAxis, xAxis ] = this.getPositions();
const isTop = 'top' === yAxis;
const isLeft = 'left' === xAxis;
const isRight = 'right' === xAxis;

let rect = anchor.parentNode.getBoundingClientRect();
if ( range ) {

This comment has been minimized.

Copy link
@aduth

aduth Oct 31, 2017

Member

Is the general idea that if we pass a range, we use it as a anchor reference instead of this.nodes.anchor ? If so, do we need to do anything else about the logic below for padding offset (should this be eliminated)?

Or, to my earlier points about making Popover unaware of range, would one of the following options work:

  • Allow parent component to pass an override for setting offset, like onSetOffset or getOffsetRect ?
  • Allow parent component to pass an anchor object which implements getBoundingClientRect (Range, Element)?

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

Is the general idea that if we pass a range, we use it as a anchor reference instead of this.nodes.anchor ?

yes

If so, do we need to do anything else about the logic below for padding offset (should this be eliminated)?

Yes it looks like it is not needed when positioned over a range.

Or, to my earlier points about making Popover unaware of range, would one of the following options work:

  • Allow parent component to pass an override for setting offset, like onSetOffset or getOffsetRect ?
  • Allow parent component to pass an anchor object which implements getBoundingClientRect (Range, Element)?

They'd probably both work to some degree though the second option would be problematic when ranges wrapped across multiple lines.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 2, 2017

Author Contributor

I have changed the calculation of the topOffset so it only happens for the anchor.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 3, 2017

Author Contributor

I have extracted the getBoundingClientRect logic into a function getAnchorRect which can be passed to override the default of positioning around the parent element.

So it now behaves like your first suggested option.

@EphoxJames

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2017

@aduth I think I have addressed all your review points hence I am merging this.

@EphoxJames EphoxJames merged commit e5e6027 into master Nov 3, 2017

3 checks passed

codecov/project 31.64% (+0.47%) compared to a092f04
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@EphoxJames EphoxJames deleted the add/2793-user-autocompletion branch Nov 3, 2017

@EphoxJames EphoxJames moved this from In Progress to Done in Ephox Team Nov 3, 2017

@@ -42,6 +42,7 @@ class Popover extends Component {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 3, 2017

Contributor

I think the changes here may have broke master. All the popovers (inserter, menus...) are centered and not relative to their parent anymore

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 6, 2017

Author Contributor

Yep, I see what you mean. I'm currently trying to figure out how my refactor changed that. Hopefully I'll have a fix for you by the time it rolls around to Europe's Monday.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 6, 2017

Author Contributor

Found the bug:

		return {
			...rect,
			top: rect.top + topPad,
			bottom: rect.bottom - bottomPad,
			height: rect.height - topPad - bottomPad,
		}

In the above ...rect does not copy across the DomRect's properties as I had expected. The fix will thankfully be simple.

This comment has been minimized.

Copy link
@EphoxJames

EphoxJames Nov 6, 2017

Author Contributor

The bug should now be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.