-
-
Notifications
You must be signed in to change notification settings - Fork 828
Conversation
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.
otherwise lgtm
@@ -21,7 +21,7 @@ import { _t } from '../../languageHandler'; | |||
import { KeyCode } from '../../Keyboard'; | |||
import sdk from '../../index'; | |||
import dis from '../../dispatcher'; | |||
import rate_limited_func from '../../ratelimitedfunc'; | |||
import { debounce } from 'lodash'; |
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.
I thought throttle
closer matched what we had? Or is there a reason to prefer the different behaviour (I think thge difference being whether it keep executing the thing every n seconds on sustained activity).
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.
From reading the documentation, debounce seems like the thing we want here with the trailing edge later defined. If someone were to roll their face on the keyboard, we probably want to debounce that rather than rate limit it.
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.
Indeed, what Travis said. Feels faster in practice as well, as the delay can be smaller.
clearTimeout(self.scheduledCall); | ||
self.scheduledCall = undefined; | ||
} | ||
import { throttle } from "lodash"; |
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.
oh, it's throttle here - is there a reason to use different ones in different places?
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.
Yeah, the behaviour we had in place here was the same as throttle, so I preserved that.
For a search field, debounce is usually better as you run the search code less and you can have a smaller delay.
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.
generally lgtm from a feature perspective - have not checked code quality, but it looks reasonable from a skim read
@@ -21,7 +21,7 @@ import { _t } from '../../languageHandler'; | |||
import { KeyCode } from '../../Keyboard'; | |||
import sdk from '../../index'; | |||
import dis from '../../dispatcher'; | |||
import rate_limited_func from '../../ratelimitedfunc'; | |||
import { debounce } from 'lodash'; |
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.
From reading the documentation, debounce seems like the thing we want here with the trailing edge later defined. If someone were to roll their face on the keyboard, we probably want to debounce that rather than rate limit it.
element-hq/element-web#8184
Also fixes/helps with element-hq/element-web#8186 as it's confusing to have the badge appear without it appearing as unread at exactly the same time.
Also replaced ratelimitedfunc in an attempt to make sure there was no bug there.
Replacing it with lodash code seems like a good thing in general though. And while at it,the search box seems to work better with debounce (doesn't run at all while still being triggered) behaviour than throttle (run every x while being triggered).