Add basic search functionality #34

GreenMan36 · 2024-12-30T15:42:31Z

It's pretty scuffed as its been a while since I worked with React.
I added the react-router-dom for the BrowserRouter so I could use useSearchParams and useNavigate.

I'd like to move more of the state to the url, this is great as it allows you to share your search queries and what snippet you have open. However, that would be a major refactor, so I'll leave the idea here.

It's been like a year but WDS had a video on it and I Theo as well. I think, I didn't re-watch to check.

GreenMan36 · 2024-12-30T15:43:24Z

I don't have much experience with react so my architecture and code is likely not so clean.

netlify · 2024-12-30T15:43:46Z

✅ Deploy Preview for quicksnip ready!

Name	Link
🔨 Latest commit	`5edc1b1`
🔍 Latest deploy log	https://app.netlify.com/sites/quicksnip/deploys/67780ca396dc7b0008cde52a
😎 Deploy Preview	https://deploy-preview-34--quicksnip.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Mathys-Gasnier · 2024-12-30T15:45:36Z

I agree that moving the state to the URL should be something we work on, as it also allows to go back and forth in your history of visited pages

src/main.tsx

src/components/SearchSnippetList.tsx

Mathys-Gasnier · 2024-12-30T15:50:58Z

As you said you aren't really familiar with react router and all, if you want I can contribute to this PR improving the way it's used and handled

GreenMan36 · 2024-12-30T16:22:52Z

As you said you aren't really familiar with react router and all, if you want I can contribute to this PR improving the way it's used and handled

I'd really appreciate that! I could figure it out, but learning React Router while working on a public repo sounds like more effort than I want to put in right now.

src/components/SnippetList.tsx

.github/ISSUE_TEMPLATE/ADD-LANGUAGE.yml

Mathys-Gasnier

For me it looks good, let's wait on another review since it's a pretty big change

kuronekony4n · 2024-12-30T23:36:26Z

you guys forget to change this thing

saminjay · 2024-12-31T02:37:47Z

src/hooks/useSnippets.ts

@@ -14,8 +14,10 @@ export const useSnippets = () => {
    `/data/${slugify(language.lang)}.json`
  );

-  const fetchedSnippets = data
-    ? data.find((item) => item.categoryName === category)?.snippets
+  const fetchedSnippets: SnippetType[] = data


instead of adding nested ternary operator, it would be better to populate it through a function.

I did the thing 🫡

saminjay · 2024-12-31T07:58:34Z

src/components/SearchInput.tsx

+    debounce((query: string) => {
+      if (query) {
+        setSearchParams({ q: query });
+        navigate(`/search?q=${encodeURIComponent(query.trim().toLowerCase())}`);


I don't think navigating urls every debounced request is a good thing. I know it is debounced but still sometimes people pause to think while searching, and it will clutter the history.

You can maybe search on Enter, or show results while typing but navigate the URL on Enter or onBlur

It's an SPA so it doesn't "refresh" the page, but yeah it will clutter the history, what you can do is use the replace feature of navigate here

I think on Enter and onBlur is a great idea! I'll look into it.

saminjay · 2024-12-31T09:17:01Z

src/App.tsx

@@ -1,29 +1,24 @@
-import { useAppContext } from "./contexts/AppContext";
-
+import { Routes, Route, BrowserRouter } from "react-router-dom";


react-router-dom is not used anywhere in the project, it was just added in package.json. So I think, it will be better to use the latest version.

I'm not sure where you're seeing it being added to the package.json. react-router-dom was part of the initial commit.
If you wish to have the dependencies updated, I recommend making a new PR for it.

Can you update it in your PR ?

It was added in the initial commit, but its not being used anywhere. Its just there.
Since you are using it in this PR, you can just make it like its being added for the first time and update it to the latest version.

Instead of using ternary operators, I made a pure function with early returns for easier readability and maintainability. Also moved the endpoint to a const for clarity/simplicity.

I rewrote this five times today, all with similar issues. Either you write too much to history, or you overwrite history too much. I'm pretty sure in the current implementation, we don't even need /search as a page

Mathys-Gasnier

Apart from the two little comments i made, it seems about right

Mathys-Gasnier · 2024-12-31T15:43:40Z

src/components/SearchInput.tsx

        setSearchParams({});
-        navigate("/");
+        navigate("/", { replace: true });


This one shouldn't replace

Mathys-Gasnier · 2024-12-31T15:45:00Z

src/components/SearchInput.tsx

+      // Only navigate if we're not already on the search page
+      if (location.pathname !== "/search") {
+        navigate("/search", {
+          replace: isCompletedSearch || location.pathname === "/search",


This one should always be false

I'll look at it next year, I've been going crazy for the past four hours trying to get this all to behave.

Mathys-Gasnier · 2024-12-31T15:47:07Z

Oh and there is a weird flicker when you go from the search page to the hompage...

I think one way to make this easier would be to remove the separation between the search and hompage...
At least from the path, and only keep it as a query param, like if ?q=thing is present then we search, else we dont.

GreenMan36 · 2024-12-31T16:03:22Z

Oh and there is a weird flicker when you go from the search page to the hompage...

I think one way to make this easier would be to remove the separation between the search and hompage...
At least from the path, and only keep it as a query param, like if ?q=thing is present then we search, else we dont.

I originally intended to make a different design, in my latest commit message I also conclude the same as you.

barrymun · 2025-01-03T15:29:47Z

When using quicksnip, I find that this is the feature I am missing most. I'm not sure about the current state of development on this PR, but I wanted to help a little, so I finished the search feature based on the work already completed and feedback within this task (addressing some of the comments and smaller errors also).

@GreenMan36 If you are currently working on this and are nearing a solution please feel free to ignore, as a lot of my work is based on yours anyway. You are more than welcome to have a look and use any code from the patch file I will attach to this comment if you find it helpful.

Here is a demo. I'm also attaching the patch file so the work can be applied to this branch. I started from a more recent version of the main branch (so it might be better to apply it after pulling latest), as a lot of new changes have been added since work on this started. Thanks all.

changes.patch

GreenMan36 · 2025-01-03T15:37:35Z

When using quicksnip, I find that this is the feature I am missing most. I'm not sure about the current state of development on this PR, but I wanted to help a little, so I finished the search feature based on the work already completed and feedback within this task (addressing some of the comments and smaller errors also).

@GreenMan36 If you are currently working on this and are nearing a solution please feel free to ignore, as a lot of my work is based on yours anyway. You are more than welcome to have a look and use any code from the patch file I will attach to this comment if you find it helpful.

Here is a demo. I'm also attaching the patch file so the work can be applied to this branch. I started from a more recent version of the main branch (so it might be better to apply it after pulling latest), as a lot of new changes have been added since work on this started. Thanks all.

changes.patch

Awesome work! I just shut down my PC but I guess imma boot it back up haha! Give me a few minutes!

I didn't share in the public channels but; https://lucaong.github.io/minisearch/

This might be a neat project to integrate.

GreenMan36 · 2025-01-03T15:57:19Z

Yikes, I merged main to mine to be up-to-date and due to all the commit hooks I had to run lint and format but that seems to have changed a couple of things?

barrymun · 2025-01-03T16:11:26Z

minisearch might be a nince addition in the future once we get the basic search up and running for sure.

barrymun · 2025-01-03T16:13:23Z

Yikes, I merged main to mine to be up-to-date and due to all the commit hooks I had to run lint and format but that seems to have changed a couple of things?

Yes, quite a lot of things have changed. Definitely make sure that all of the packages are up to date as well. Linting and prettier formatting might require you to change some things as this PR is based on older code.

GreenMan36 · 2025-01-03T16:27:28Z

Yeah I'm having major issues with Prettier, ESlint and GIT all messing with formatting and line styles and whatnot. I think this branch and all my other are now broken.

barrymun · 2025-01-03T16:29:42Z

Yeah I'm having major issues with Prettier, ESlint and GIT all messing with formatting and line styles and whatnot. I think this branch and all my other are now broken.

Maybe the easiest thing to do might be to keep this PR active for now, create a new branch from latest and get everything working, apply the patch from above then make any changes you need from this branch? Maybe create a new PR as well if you need to more easily compare the changes.

GreenMan36 · 2025-01-03T16:35:36Z

I did a hard reset of my branch on main, and its still broken. I recommend you make a new branch with your changes, make a new PR and you can mention this PR.

Your changes look good to me.

barrymun · 2025-01-03T16:54:26Z

@GreenMan36 The new PR is here #159. Please feel free to leave a review, or suggest/make any changes that you think I've missed. Thanks

saminjay · 2025-01-09T13:10:09Z

Closing this PR.
As a new one is already raised and author said its broken now.

GreenMan36 added 2 commits December 30, 2024 16:05

Add basic search functionality

7c572c2

Move browser router to app.tsx

3474894

Consolidated snippet and search snippet list

b91a02f

Mathys-Gasnier requested changes Dec 30, 2024

View reviewed changes

src/main.tsx Outdated Show resolved Hide resolved

src/components/SearchSnippetList.tsx Outdated Show resolved Hide resolved

Mathys-Gasnier reviewed Dec 30, 2024

View reviewed changes

src/components/SnippetList.tsx Outdated Show resolved Hide resolved

Mathys-Gasnier added the update needed Code needs to be updated. label Dec 30, 2024

Add description to snippetlist and limit title & desc to 1 line

6a44981

Mathys-Gasnier reviewed Dec 30, 2024

View reviewed changes

.github/ISSUE_TEMPLATE/ADD-LANGUAGE.yml Outdated Show resolved Hide resolved

Remove straggler issue template file

53be0bd

Mathys-Gasnier approved these changes Dec 30, 2024

View reviewed changes

saminjay reviewed Dec 31, 2024

View reviewed changes

Refactor useSnippets with feedback and more

7031525

Instead of using ternary operators, I made a pure function with early returns for easier readability and maintainability. Also moved the endpoint to a const for clarity/simplicity.

dostonnabotov mentioned this pull request Dec 31, 2024

[Bug] - search bar does not work #64

Open

Add less history spammy but scuffed search

5edc1b1

I rewrote this five times today, all with similar issues. Either you write too much to history, or you overwrite history too much. I'm pretty sure in the current implementation, we don't even need /search as a page

Mathys-Gasnier reviewed Dec 31, 2024

View reviewed changes

saminjay linked an issue Jan 1, 2025 that may be closed by this pull request

[Bug] - search bar does not work #64

Open

GreenMan36 force-pushed the basic-search branch from 2604786 to 5edc1b1 Compare January 3, 2025 16:13

barrymun mentioned this pull request Jan 3, 2025

Feature - Implement basic search functionality v2 #159

Open

10 tasks

saminjay closed this Jan 9, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic search functionality #34

Add basic search functionality #34

GreenMan36 commented Dec 30, 2024

GreenMan36 commented Dec 30, 2024

netlify bot commented Dec 30, 2024 •

edited

Loading

Mathys-Gasnier commented Dec 30, 2024

Mathys-Gasnier commented Dec 30, 2024

GreenMan36 commented Dec 30, 2024

Mathys-Gasnier left a comment

kuronekony4n commented Dec 30, 2024

saminjay Dec 31, 2024

GreenMan36 Dec 31, 2024

saminjay Dec 31, 2024

Mathys-Gasnier Dec 31, 2024

GreenMan36 Dec 31, 2024

saminjay Dec 31, 2024

GreenMan36 Dec 31, 2024

Mathys-Gasnier Dec 31, 2024

saminjay Dec 31, 2024 •

edited

Loading

Mathys-Gasnier left a comment

Mathys-Gasnier Dec 31, 2024

Mathys-Gasnier Dec 31, 2024

GreenMan36 Dec 31, 2024

Mathys-Gasnier commented Dec 31, 2024

GreenMan36 commented Dec 31, 2024

barrymun commented Jan 3, 2025 •

edited

Loading

GreenMan36 commented Jan 3, 2025

GreenMan36 commented Jan 3, 2025

barrymun commented Jan 3, 2025

barrymun commented Jan 3, 2025

GreenMan36 commented Jan 3, 2025

barrymun commented Jan 3, 2025

GreenMan36 commented Jan 3, 2025

barrymun commented Jan 3, 2025

saminjay commented Jan 9, 2025

		@@ -1,29 +1,24 @@
		import { useAppContext } from "./contexts/AppContext";

		import { Routes, Route, BrowserRouter } from "react-router-dom";

Add basic search functionality #34

Add basic search functionality #34

Conversation

GreenMan36 commented Dec 30, 2024

GreenMan36 commented Dec 30, 2024

netlify bot commented Dec 30, 2024 • edited Loading

✅ Deploy Preview for quicksnip ready!

Mathys-Gasnier commented Dec 30, 2024

Mathys-Gasnier commented Dec 30, 2024

GreenMan36 commented Dec 30, 2024

Mathys-Gasnier left a comment

Choose a reason for hiding this comment

kuronekony4n commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saminjay Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Mathys-Gasnier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mathys-Gasnier commented Dec 31, 2024

GreenMan36 commented Dec 31, 2024

barrymun commented Jan 3, 2025 • edited Loading

GreenMan36 commented Jan 3, 2025

GreenMan36 commented Jan 3, 2025

barrymun commented Jan 3, 2025

barrymun commented Jan 3, 2025

GreenMan36 commented Jan 3, 2025

barrymun commented Jan 3, 2025

GreenMan36 commented Jan 3, 2025

barrymun commented Jan 3, 2025

saminjay commented Jan 9, 2025

netlify bot commented Dec 30, 2024 •

edited

Loading

saminjay Dec 31, 2024 •

edited

Loading

barrymun commented Jan 3, 2025 •

edited

Loading