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

fix: add debounced data-element search #88

Draft
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

Birkbjo
Copy link
Member

@Birkbjo Birkbjo commented Jul 6, 2022

As an extension of #87 , I wanted to make the search smoother. That PR already solves a lot of the lag experienced when searching, and this should help it further.

Note how the controlled value is now in a separate component. This should help re-renders in eg. section.js, since previusly the entire Section was re-rendered for each key-stroke. Now this is only re-rendered after the debounce.

Introduced a new hook useDebounceCallback, which basically is useDebounce in combination of using the debounced-value in an useEffect to call a callback when the debounce is "done".

Could also refactor debounced-search-input.js to use this common hook. But didn't want to touch more than necessary here.

Note: Want to merge #87 before this, as that contains optimisations that this PR benefits from. However, these shouldn't have conflicts, but if you check out this PR the performance is not that great. Both together works really well.

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Jul 6, 2022

🚀 Deployed on https://pr-88--dhis2-data-entry.netlify.app

Copy link
Collaborator

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

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

This is the first time I took a closer look at useDebounce, so what I'll say now is not your fault at all and I can see why you implemented things the way they are due to what's been there already.

Having said that, I think that the names useDebounce and useDebounceCallback are misleading and I'd like to start at least a discussion about changing some things here:

  1. debounce is a name commonly used for debouncing a function call. I'd expect useDebounce to accept a function (A) and return a function B. Then I'd expect that function A will be debounced by a certain amount of time when calling function B, just like using lodash's debounce function, but in a react context
  2. useDebounceCallback's name is very similar to useCallback, so I'd also expect this to be functionally similar to a regular debounce function but in a react context

I think, if we were to keep the existing implementation, we should:

  • rename useDebounce to useDebouncedValue, that would make it very clear.
  • rename useDebounceCallback to useDebounceCallbackWithValue. I think it's very hard to find a highly accurate name for this hook, but useDebounceCallback is "wrong" I think.

I have another suggestion:
Let's say we implement a useDebounce that is a hook-wrapper for lodash's debounce function and use that to debounce the setGlobalFilterText function as this is where we want to debounce conceptually. The filter field doesn't need to know about that implementation detail plus it'd be more portable / re-usable. And whether the debounce mechanism should be used depends on the part that uses the filter field.

The filter field would then simply need the following function for the onChange props:

const onChange = ({ value }) => {
    setFilterText(value)
    onFilterChange(value)
}
// ...
    <InputField /* ... */ onChange={onChange} /> 
    <Button /* ... */ onChange={onChange} />
// ...

This suggestion seems much clearer to me conceptually and is more future-proof as the <FilterField /> is more portable/reusable and the debounce mechanism is placed where we actually need it conceptually. On top of that we wouldn't have to use useDebounceCallback, for which I can't find a proper descriptive name that's not too long.

@Mohammer5
Copy link
Collaborator

By @Birkbjo:

will come back to this, but pretty low priority, think it’s something we might want, but after refactor to display: none its very fast as is

@stale
Copy link

stale bot commented Apr 19, 2023

Hi!

Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open.

Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖

@stale stale bot added the stale label Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants