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

Feature - Implement basic search functionality v2 #159

Merged
merged 25 commits into from
Jan 29, 2025

Conversation

barrymun
Copy link
Contributor

@barrymun barrymun commented Jan 3, 2025

Description

  • These changes will allow the search bar to work as intended.
  • Changes in the PR are heavily inspired by Add basic search functionality #34 and the work completed by @GreenMan36 and the other comments/suggestions within that PR.
  • A demo can be found here.

Type of Change

  • ✨ New snippet
  • 🛠 Improvement to an existing snippet
  • 🐞 Bug fix
  • 📖 Documentation update
  • 🔧 Other (please describe): Feature - implementing basic search functionality

Checklist

  • I have tested my code and verified it works as expected.
  • My code follows the style and contribution guidelines of this project.
  • Comments are added where necessary for clarity.
  • Documentation has been updated (if applicable).
  • There are no new warnings or errors from my changes.

Related Issues

Closes #64 #56 #224

Additional Context

Screenshots (Optional)

Click to view screenshots

Copy link

netlify bot commented Jan 3, 2025

Deploy Preview for quicksnip ready!

Name Link
🔨 Latest commit bfe0b4b
🔍 Latest deploy log https://app.netlify.com/sites/quicksnip/deploys/677816022c5c3e0008d11d4b
😎 Deploy Preview https://deploy-preview-159--quicksnip.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@dostonnabotov
Copy link
Owner

Hey, @barrymun. Great work. 👍 After seeing the demo, I noticed that there is also All snippets category. Will we have this in the final version as well? 🤔

@barrymun
Copy link
Contributor Author

barrymun commented Jan 3, 2025

@dostonnabotov This feature was in v1 (#34) and I thought it was to be implemented here too?

Copy link
Collaborator

@Mathys-Gasnier Mathys-Gasnier left a comment

Choose a reason for hiding this comment

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

Seeing as you changed a lot of things already, could you maybe add the language + category path to the url ? And make snippets link shareable to fix #56

IMO we should to it like so:

  • / Default lang + default category
  • /javascript Lang + default category
  • /javascript/category Lang + Category
  • *?q=search Searching in, * being the lang, category or lack of, in which you want the search to happen
  • *?snippet=snippet-name Opens the snippet preview, * has the same behavior as q=, both can be found at the same time to preserve search too

And we should be able to reconstruct the page based on that, for example:
quicksnip.dev/javascript/basics?q=hello&snippet=hello-world Should open quicksnip on the javascript language, in the basics category, with a search of hello and the hello-world snippet opened

src/router.tsx Outdated Show resolved Hide resolved
@dostonnabotov
Copy link
Owner

@barrymun I see that you've been actively participating on improving QuickSnip. It would be awesome if you could join our Discord server. I will give you access to our maintainers channel, in which we discuss ideas and plans for QuickSnip. You have been really helpful for our project.

@barrymun
Copy link
Contributor Author

barrymun commented Jan 3, 2025

@barrymun I see that you've been actively participating on improving QuickSnip. It would be awesome if you could join our Discord server. I will give you access to our maintainers channel, in which we discuss ideas and plans for QuickSnip. You have been really helpful for our project.

@dostonnabotov Thank you, I'm already a member, and my username should be the same as it is here (barrymun).

@barrymun
Copy link
Contributor Author

barrymun commented Jan 4, 2025

I will be away until next week but I think I have a decent solution for this, and will hopefully have the code ready shortly after I return.

@majvax majvax added the enhancement New feature or request label Jan 4, 2025
Copy link
Collaborator

@Mathys-Gasnier Mathys-Gasnier left a comment

Choose a reason for hiding this comment

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

Looks good to me

…L query param and not the search text, remove the searchVal state and just use the AppContext searchText state instead as it is cleaner, ensure that the search term is preserved when switching categories should one exist
@barrymun
Copy link
Contributor Author

barrymun commented Jan 10, 2025

Made some additional changes to fix some small bugs and refactored the code a little so that it is cleaner, but it should be ready now.

@Mathys-Gasnier
Copy link
Collaborator

Lets wait on another review and we are good to go

@barrymun
Copy link
Contributor Author

Do you know if it's possible to point the netlify deploy to the latest commit for this branch? It still seems to be referencing the commit from last week and it would make it easier for others to test if it were updated.

@Mathys-Gasnier
Copy link
Collaborator

We disabled auto previews because we were running out of build time with our current plan, they will need to clone and test localy

@saminjay
Copy link
Collaborator

It works great but there are few minor issues,

  • When you first open the website, it will show JavaScript -> Array Manipulation but will not update the url. I think you should redirect the default path / to javascript/array-manipulation or something else.
  • When we are on the root path / and open the snippet it works correctly, it is even sharable. But does not add javascript/array-manipulation to the url bar. Its just the first issue.
  • The search is not working live. You have to press enter to search, I think it will be better to show the results while typing and only change the url on enter.
  • Also I think search should also update on blur.

@Mathys-Gasnier
Copy link
Collaborator

I'm not a big fan of the site redirecting / to javascript/array-manipulation... I think it clutters the URL when you open the website...

One option would be to, in the futur have a real homepage, with just the selection of languages

@saminjay
Copy link
Collaborator

I'm not a big fan of the site redirecting / to javascript/array-manipulation... I think it clutters the URL when you open the website...
One option would be to, in the futur have a real homepage, with just the selection of languages

We need something though, It doesn't make any sense to have one category on / and all the other on their respective routes. Redirecting or replacing feels like a good alternative until we add a dashboard or home or something.

@barrymun
Copy link
Contributor Author

To be honest, I don't like the idea of a popup/global search, in most cases you are searching for a snippet in a specific language, so just having a "default" category, which would contain every snippet, would make searching in a language easier.

Do you think it would be better to revert the latest change, add this "default" category which would be "All Snippets" (or equivalent) and this would show all snippets for the selected language?

@dostonnabotov
Copy link
Owner

To be honest, I don't like the idea of a popup/global search, in most cases you are searching for a snippet in a specific language, so just having a "default" category, which would contain every snippet, would make searching in a language easier.

I see your point. searching in a specific language would be easier. I was also thinking like search should apply globally. For example, when I land on a website and I need to find "flatting a list in Python", I wouldn't need to first go to Python itself to be able to search it. I feel like in terms of UX, user should be able to find any snippet with search, or at least, that's what he expects for the first time.

And if we implement it like this, it would be better to keep the search param in the url between categories navigation, so the user is able to filter more using the already existing and familiar interface

With above approach, I think filters would be more useful in the future as we could use them to filter languages as well.

Let me know your thoughts on this approach. It's always good to discuss various perspectives.

…ages, categories and snippets"

This reverts commit 31e622c.
…his is the default category for any langauge and will show all the snippets for that language.
@barrymun
Copy link
Contributor Author

@dostonnabotov @Mathys-Gasnier

I've reverted the most recent commit which added global search and reworked the search feature again to include a default category (for all languages) which is labelled All Snippets and shows all the snippets for the currently selected language. Let me know your thoughts.

@Mathys-Gasnier
Copy link
Collaborator

IMO adding a popup/search page is the thing that would complexify the UI, and doing a search capable of understanding things like in Python is a real pain without using external services...
I don't think we need a really big search system, the project is small enough where tags and descriptions are enough for basic search.

Searching globally serves no point, like if I want to find python snippets, why should i bother writting "in Python" to not have all the clutter around it when there is already something to do exactly that in the UI

Copy link
Collaborator

@psychlone77 psychlone77 left a comment

Choose a reason for hiding this comment

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

Changes look good.
Some improvements for future could be, adding a NotFound page, and improving SEO capabilities to this site.

@dostonnabotov
Copy link
Owner

IMO adding a popup/search page is the thing that would complexify the UI, and doing a search capable of understanding things like in Python is a real pain without using external services...

I was also thinking using external library to do the heavy lifting. Algolia for Open-Source has very generous offers for open source projects. Since it takes care of search stuff, we can focus on our other upcoming features. Let me know your thoughts.

@barrymun
Copy link
Contributor Author

I was also thinking using external library to do the heavy lifting. Algolia for Open-Source

Algolia (even though it would be a nice addition) might be overkill at this stage?

@Mathys-Gasnier
Copy link
Collaborator

I think an external lib for a project like quicksnip is way overkill

@psychlone77
Copy link
Collaborator

As discussed in #34, we could use https://lucaong.github.io/minisearch/ if needed in the future, for more advanced search results.

@dostonnabotov
Copy link
Owner

Since many are agreeing on not using external lib, we can continue with our own search functionality.

And, regarding the current implementation, only searching within a specific language doesn't appeal to me. It would be awesome to implement the global search. Perhaps, we might have to consolidate all files in a single file.

For example, in MDN (https://developer.mozilla.org/en-US/) website, you don't first select the language, then search things within it. You just search and it shows everything in the documentation. If it's a lot of headache to implement, we could at least use Minisearch, https://lucaong.github.io/minisearch/ to handle it.

I would like to hear your thoughts on this.

@barrymun
Copy link
Contributor Author

This commit generates an “all.json” file which allows the user to search through every language and category uploaded using a global search. Stylistically it is closer to GitHub as opposed to Mozilla. No minisearch required as of yet. On local make this commit the head and see if you prefer this implementation.

@Mathys-Gasnier
Copy link
Collaborator

I know that things like modzilla do it globally, but they are a documentation...
We are a snippet library, when going on quicksnip, if when i search "fibona" it returns me ALL fibonacci snippets, i'm just going to be lost, adding this we are only complexifying the interface for something not really usefull/really niche

@dostonnabotov
Copy link
Owner

This commit generates an “all.json” file which allows the user to search through every language and category uploaded using a global search. Stylistically it is closer to GitHub as opposed to Mozilla. No minisearch required as of yet. On local make this commit the head and see if you prefer this implementation.

I like this implementation. It looks closely to what we were discussing in Discord. I noticed routing system is kinda interesting:

/javascript/array-manipulation?q=arra&snippet=partition-array

Seeing as you changed a lot of things already, could you maybe add the language + category path to the url ?

I believe snippet sharing and searching are mixing up together. Perhaps, we could remove the searching query from the url and make the snippet sharing on its own? While searching could be managed with state or context api. @Mathys-Gasnier and @barrymun, let me know your thoughts.

/javascript/array-manipulation/partition-array

@Mathys-Gasnier
Copy link
Collaborator

IMO we should keep it as is, it allows for sharing snippets, but also searchs, for me, it happens often that i want to share something with the search, especially with beginners, to show them how to search for something, this would allow to share searchs to multiple snippets

@barrymun
Copy link
Contributor Author

@Mathys-Gasnier how do you want to handle the new subLanguages feature w.r.t. sharing and searching?

@Mathys-Gasnier
Copy link
Collaborator

honestly, don't search too far, just add it to the url javascript/react/basics, if a sublanguage match we go with the sub language, if not we search a category matching the name, for searching it should behave as a normal language for the moment

@barrymun
Copy link
Contributor Author

Sharing has now been reworked to handle the new sub language selection.

@dostonnabotov
Copy link
Owner

Hey, there. Awesome work xD. Will review it.

@dostonnabotov dostonnabotov merged commit 94cf03c into dostonnabotov:main Jan 29, 2025
1 check passed
@barrymun barrymun deleted the feature/search branch January 29, 2025 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] - search bar does not work
7 participants