Stop registering SuggestionListElement as a custom element#826
Stop registering SuggestionListElement as a custom element#826
Conversation
...so that all the files belonging to this package can be snapshotted.
| editorElement.classList.add('autocomplete-active') | ||
| } | ||
|
|
||
| process.nextTick(() => { this.suggestionListElement.didAttach() }) |
There was a problem hiding this comment.
@joefitzgerald: what do you think about using process.nextTick here? Removing custom elements also prevents us from detecting when the element has been attached. In this case, we will simply signal SuggestionListElement that it has been attached on the next tick of the event loop.
There was a problem hiding this comment.
I'm not sure - are there any negative implications?
There was a problem hiding this comment.
I don't think so, but ideally we would do this in a atom.views.updateDocument callback. I guess it should be fine, because we still schedule an animation frame as part of render() which is called in didAttach().
|
I was able to see auto complete working, but when I tested on js files before and after and saw different results pop up. see screenshot: |
|
Interesting, this shouldn't change the behavior of the suggestion list. Did you have a set of different files open in the second screenshot? |
|
AH, sorry, I think I did, when I have the same files open I see the same. I didn't realize that was what taken to account. I didn't see any issues after this clarification. Thanks! LGTM |
| } | ||
|
|
||
| showAtBeginningOfPrefix (editor, prefix, followRawPrefix = false) { | ||
| if (!editor) { |
There was a problem hiding this comment.
Isn't an early return with no nesting of meaningful code preferable to the alternative in this function?
| const marker = this.suggestionMarker = editor.markBufferRange([bufferPosition, bufferPosition]) | ||
| this.overlayDecoration = editor.decorateMarker(marker, {type: 'overlay', item: this, position: 'tail'}) | ||
| this.addBindings(editor) | ||
| if (editor) { |
There was a problem hiding this comment.
This nesting is unnecessary if we exit early when editor is falsy.
There was a problem hiding this comment.
Not really, because we need to call destroyOverlay if the editor is not the same. I guess we could have an early return that calls destroyOverlay, but I'm not sure if that would be clearer.
| editorElement.classList.add('autocomplete-active') | ||
| } | ||
|
|
||
| process.nextTick(() => { this.suggestionListElement.didAttach() }) |
There was a problem hiding this comment.
I'm not sure - are there any negative implications?
| return atom.views.readDocument(this.readUIPropsFromDOM.bind(this)) | ||
| } | ||
|
|
||
| addActiveClassToEditor () { |
There was a problem hiding this comment.
I'm trying to recall why we needed to add this. Why are we able to remove it now?
There was a problem hiding this comment.
Because we add the active class to the editor above in the call stack, in the showAtBeginningOfPrefix and showAtCursorPosition methods, where we also create the overlay decoration.
|
Thank you @joefitzgerald and @ungb! ❤️ I think I will go ahead and merge this and let it can bake for a while on master. |
6 participants
This pull request stops registering custom elements during startup and uses simple javascript objects that internally manage a DOM element instead.
/cc: @ungb for 👀