-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Migrate AddToDashboard dialog #4408
Migrate AddToDashboard dialog #4408
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.
Looks good, only thing I missed was a loading indicator (mainly for when it is not that fast to load - which was my case testing it on Preview 😅)
import { useState, useEffect } from 'react'; | ||
import { useDebouncedCallback } from 'use-debounce'; | ||
|
||
export default function useSearchResults( |
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.
Cool 👍, I will keep it in mind for next search uses
const [searchTerm, setSearchTerm] = useState(''); | ||
|
||
const [doSearch, dashboards, isLoading] = useSearchResults((term) => { | ||
if (isString(term) && (term.length >= 3)) { |
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.
The loading indicator was not the only thing that caused me a bit of confusion 😕, do we really need this term.length >= 3
? (I mean, the results are paged, right?) There is the option of using Antd form to indicate that minimum length, but I don't think it's that worth it. Well, wdyt?
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'm not sure why it was implemented this way; of course that API can search by a single character. Also, here we don't show all results, but only a most relevant ones (20 or 25 - don't remember exactly) - I think it makes sense.
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 think the idea was to reduce number of requests we send because it's less likely that a <3 characters term will be meaningful. We can drop 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.
(The dragged parameters spec is flaky, will fix that in a few minutes) |
What type of PR is this? (check all applicable)
Mobile & Desktop Screenshots/Recordings (if there are UI changes)