-
-
Notifications
You must be signed in to change notification settings - Fork 829
Conversation
… feature-autocomplete
@@ -5,7 +5,8 @@ import { | |||
convertFromHTML, | |||
DefaultDraftBlockRenderMap, | |||
DefaultDraftInlineStyle, | |||
CompositeDecorator | |||
CompositeDecorator, | |||
SelectionState |
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.
Really minor thing, but if we're moving to all-new ES2016, presumably this can end with a comma?
Looking good! |
Can one of the admins verify this patch? |
@matrixbot test this please |
this.setState({ | ||
completions: newCompletions | ||
completions: newCompletions, | ||
completionList: _.flatMap(newCompletions, provider => provider.completions), |
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.
Is it worth pulling in all of lodash just for this?
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.
Lodash has a bunch of useful utilities which can replace a decent amount of
code that's currently in the codebase. ratelimitedfunction, for example.
On Mon 4 Jul, 2016, 4:13 PM David Baker, notifications@github.com wrote:
In src/components/views/rooms/Autocomplete.js
#296 (comment)
:this.setState({
completions: newCompletions
completions: newCompletions,
completionList: _.flatMap(newCompletions, provider => provider.completions),
Is it worth pulling in all of lodash just for this?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/matrix-org/matrix-react-sdk/pull/296/files/a74db3a815879558110fcaa4b2f6f322dc1af783..cccc58b47f77f0eec644bc2455916496ed468318#r69440073,
or mute the thread
https://github.com/notifications/unsubscribe/AAOlRHqTcDZbZTyo-RK80t7msI-bmPF_ks5qSOPBgaJpZM4Irc8R
.
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.
OK - not opposed in principle, just want to make sure it's worth an extra 22kB on the bundle
Trying it out, I get the autocomplete list every other character I type, ie. I type an '@' symbol and the list of users appears, then I type 't' and it disappears, I type 'e' and it appears again. |
Also, it would be good if the 4 emoji that appear when you type a colon were commonly used things like the smilies: having it pop up with some kissing couples and a fist is somewhat offputting :p |
It also seems to get a little confused with the colons in room aliases / user IDs, popping up the emoji autocomplete. |
OK, looking good in general, just a few comments as above. |
That it alternates between showing and not showing completions when typing seems to be a recent regression -- did it work for you before the last update (ie. before replacement was added?) |
Re: the alternating, unsure to be honest, as far as I can remember, it did. |
OK, lgtm! |
Still WIP. Fixes element-hq/element-web#1574, element-hq/element-web#821, element-hq/element-web#644, element-hq/element-web#584, element-hq/element-web#883, element-hq/element-web#572.
TODO: