-
Notifications
You must be signed in to change notification settings - Fork 146
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
BED-5445: Deep linking node search #1182
base: main
Are you sure you want to change the base?
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.
This looks great!! I like the organization and how you did useNodeSearch
I left one comment on a potential solution for the missing adornment! But if we determine its not feasible/doesnt make sense im happy to stamp this as is
// Watch query params for a new incoming node search and sync to internal state | ||
useEffect(() => { | ||
if (primarySearch) { | ||
setSearchTerm(primarySearch); | ||
} | ||
}, [primarySearch, searchType]); |
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.
Nice!! This will be useful for when we update primarySearch outside of the normal input but we want to make sure the input is synced to that param
/* Reusable logic for syncing up a single node search field with browser query params on the Explore page. The value of the search field is tracked internally, and is only pushed to query params once the event handler is called by the consumer component. Direct changes to the associated query params will be synced back to the search field. */ | ||
export const useNodeSearch = () => { | ||
const [searchTerm, setSearchTerm] = useState<string>(''); | ||
const [selectedItem, setSelectedItem] = useState<SearchValue | undefined>(undefined); |
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 see what youre saying about the adornment missing on initial render... definitely not the worst thing but I have an idea:
I think the selectedItem
could be derived from the searchTerm
. From what I can tell, its intention in <ExploreSearchCombobox />
appears to be to display the type adornment and thats it (current behavior). If we were using it to display the selectedItem
in the list of nodes, I don think it would be possible to derive from searchTerm
.
So if all we need is the node type which matches the inputs value then I think its safe to say that if a search term returns a single node, then we can display that nodes type?? In which case we could remove the selectedItem prop and say:
// selectedItem could be renamed to startAdornment or somethin
const selectedItem = data?.length === 1 ? data[0] : null;
what are your thoughts on thus?
Description
Adds single node search functionality to our deep linking implementation on the Explore page. This feature can be tested by enabling the
back_button_support
feature flag. Some other aspects of the Explore page (particularly pathfinding search) may appear temporarily broken when this flag is enabled.Motivation and Context
First search type implemented in the larger deep linking initiative.
This PR addresses: BED-5455
How Has This Been Tested?
Tests have been added to cover the
useNodeSearch
hook that wraps the new query param interactions.Screenshots (optional):
Types of changes
Checklist: