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.

Sign up for GitHub

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

Jump to bottom

feat: nested search #105

Merged
merged 19 commits into from
Nov 5, 2021
Merged

feat: nested search #105

merged 19 commits into from
Nov 5, 2021

Conversation

timc1
Copy link
Owner

@timc1 timc1 commented Oct 29, 2021 •
edited
Loading

Closes #5.

Nested search begins only when a search query is present.

Screen Shot 2021-10-28 at 10 38 41 PM

@vercel
Copy link

vercel bot commented Oct 29, 2021 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/timc/kbar/GhqzoSSjFXxoNTQWBQ7x61f7rDdP
✅ Preview: https://kbar-git-feat-nested-search-timc.vercel.app

@timc1 timc1 force-pushed the feat/nested-search branch from f64d821 to 066250e Compare October 29, 2021 02:39
timc1
timc1 commented Oct 29, 2021
View reviewed changes
src/useDeepMatches.tsx Outdated Show resolved Hide resolved
@timc1 timc1 force-pushed the new-actions branch 3 times, most recently from d1f2a11 to 6314bcc Compare October 29, 2021 19:12
@timc1 timc1 force-pushed the feat/nested-search branch from 066250e to fb976f9 Compare October 29, 2021 19:14
@timc1 timc1 force-pushed the feat/nested-search branch from fb976f9 to 3665431 Compare October 29, 2021 19:14
tommoor
tommoor reviewed Oct 30, 2021
View reviewed changes
src/KBarResults.tsx
Comment on lines +87 to +89
// ensure that if the first item in the list is a group
// name and we are focused on the second item, to not
// scroll past that group, hiding it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@timc1 timc1 force-pushed the feat/nested-search branch from c1a97b2 to 6508290 Compare November 1, 2021 20:36
@timc1 timc1 changed the base branch from new-actions to main November 2, 2021 01:32
timc1
timc1 commented Nov 2, 2021
View reviewed changes
src/useStore.tsx
@@ -19,14 +19,18 @@ export default function useStore(props: useStoreProps) {
);
}

const actionsInterface = React.useRef(new ActionInterface(props.actions));
Copy link
Owner Author

@timc1 timc1 Nov 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This was running new ActionInterface on each render 🤦🏻‍♂️ we can either lazily initialize with useState or useMemo.

@timc1 timc1 force-pushed the feat/nested-search branch from 35d2ca7 to 9c944df Compare November 2, 2021 02:24
@timc1 timc1 force-pushed the feat/nested-search branch from 9c944df to 7d4a33e Compare November 2, 2021 02:28
@timc1 timc1 force-pushed the feat/nested-search branch from 7d4a33e to 4df68a0 Compare November 2, 2021 02:29
@timc1 timc1 force-pushed the feat/nested-search branch from 1593cb5 to ef339b5 Compare November 2, 2021 20:41
@timc1 timc1 marked this pull request as ready for review November 2, 2021 20:45
@timc1 timc1 requested a review from tommoor November 2, 2021 20:45
@timc1 timc1 force-pushed the feat/nested-search branch from ef339b5 to 4d3a49b Compare November 2, 2021 23:12
tommoor
tommoor reviewed Nov 3, 2021
View reviewed changes
example/src/App.tsx
},
ref: React.Ref<HTMLDivElement>
) => {
const ancestors = React.useMemo(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could ancestors not be provided by onRender ?

Copy link
Owner Author

Choose a reason for hiding this comment

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

We can but we will get this behavior, notice the slight bit of ancestor flash when we hit backspace and go to the parent action:

Kapture.2021-11-03.at.13.54.08.mp4

Since we throttle the matching, the actions displayed and the currentRootActionId will not be synced perfectly. The current solution is to return the currentRootActionId alongside the matches so we ensure they're synced:

const { results, rootActionId } = useDeepMatches();

I may be missing something totally here bc I really don't like returning that rootActionId as part of useDeepMatches lol

@timc1 timc1 requested a review from tommoor November 4, 2021 15:50
@timc1 timc1 merged commit eacf193 into main Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tommoor tommoor Awaiting requested review from tommoor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add ability to search for nested content
2 participants
@timc1 @tommoor