Skip to content

feat: Search via URL - Add q and qd parameters #988

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

Closed
wants to merge 12 commits into from

Conversation

amenk
Copy link
Contributor

@amenk amenk commented Apr 13, 2025

Description

See #119

  • Introduced ?q= parameter which prefills the search and opens the search dropdown
  • Introduce ?qd= parameter (qd for query directly) which searches for the first result.
  • URL is updated (debounced) when typing a search query

Disclaimer: I did this with cursor AI - feel free to discard completely.

Mainly doing the MR to have some PoC Vercel app for further testing.

Example links

Screenshots

Checklist

  • dark mode / light mode
  • mobile / desktop
  • server-side-rendering (SSR)
  • all texts are localized (in vocabulary.ts)

Copy link

vercel bot commented Apr 13, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
osmapp ✅ Ready (Inspect) Visit Preview Apr 13, 2025 6:47pm

@amenk
Copy link
Contributor Author

amenk commented Apr 13, 2025

Regression bugs:

  • when clicking an result in a the quick search it no longer closes
  • when using qd and then clicking a node on the map, the search opens

@amenk amenk changed the title Search via URL - Add q and qd parameters feat: Search via URL - Add q and qd parameters Apr 13, 2025
@amenk
Copy link
Contributor Author

amenk commented Apr 13, 2025

bugs should be fixed.

@zbycz
Copy link
Owner

zbycz commented Apr 15, 2025

Wow, great that you tried AI coding. Overall it looks good, i have to take a closer look on desktop.

I tried once Github copilot AI coding, but i wasnt patient enough to pass over the issues, since it was easier to do it myself 😃

@zbycz
Copy link
Owner

zbycz commented Apr 16, 2025

I tried to untangle the code for less than an hour, but it is too hard :) The issue is, that current code is already getting old, and it needs some urgent refactorings (like "await fetchGeocoderOptions" which in turn could be called directly from "qd" useEffect). Of course, the AI also makes some unnecessary changes like removing comments, moving inputRef outside for no reason etc, but these quite easy to revert. The architecture though is harder.

Merging code like this would make any further refactorings impossible. I have already seen it in companies I worked for – the product team forces some features, there is no time for proper refactoring and the code debt becomes unmanageable.

Though, the good point is that I had to go over the SearchBox code and i see more clearly where are the issues. 🙂 🤞

Also thanks for testing the Cursor AI for me, it is intriguing to see how it works. (And where it doesn't work well) ❤️

@amenk
Copy link
Contributor Author

amenk commented Apr 16, 2025

Yeah, thanks to look into that. I expected something like this.
While I prompted to refactor as you suggested in the original issue, it looks like it did not do this.

Hope you find time to refactor it, soon :-)

@amenk amenk closed this Apr 16, 2025
@amenk
Copy link
Contributor Author

amenk commented Apr 18, 2025

@zbycz I noticed you started the refactoring. Very nice. 👍👀👍

@zbycz
Copy link
Owner

zbycz commented Apr 19, 2025

@amenk - yep, i am already polishing the ?qd= PR. After the refactoring, the code is so simple.

And also - I have to admit, that I used part of this PR as a reference. It was much easier for me to imagine how it all should work, because I had the glimpse of verbose-but-working solution. So - if you ever needed some new feature in OsmAPP, using cursor to make a prototype is a good idea after all! 👍🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants