-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
SearchBox: Refactoring and searchable stars #552
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…ox-types # Conflicts: # src/components/SearchBox/SearchBox.tsx # src/components/SearchBox/options/geocoder.tsx # src/components/SearchBox/renderOptionFactory.tsx # src/components/SearchBox/utils.tsx # src/services/mapStorage.ts
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.
Wow, i am very pleasantly surprised. You executed the refactoring of Options in the same manner I was planning it myself. As if you read my mind! 👍 😅
Since I merged the DirectionBox #515 in the meantime, which uses a lot of Option
, I refactored it here as well. Also it was easier for me to resolve the conflicts.
Comments are more like ideas, it is ok to merge this as is, and fix comments eg. in followup.
.map((star) => ({ | ||
star, | ||
// TODO matching is not optimal, maybe Sørensen–Dice coefficient | ||
// https://www.npmjs.com/package/dice-coefficient |
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.
Also levenstein distance could be an alternative for shorter strings.
Main issue I have with the current "match" function is, that if I search eg "pr ca" I would expect "prague castle" to be at the top, but it looks as if input words are matched with OR logical fn, even though I would expect AND.
In short, match all words has more priority, than matching words with typos.
Sometimes we could extract matching to some generic fn, so it works consistently both for presets and stars.
abortableQueueName: GEOCODER_ABORTABLE_QUEUE, | ||
}); | ||
})) as { features: GeocoderOption['geocoder'][] }; |
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.
It would feel more straight forward, if we started with type PhotonResponse
and derive the GeocoderOption
from it.
Ideally i would expect a line like this:
const searchResponse = await fetchJson<PhotonResponse>(url, options);
If you agree, you may do it in this PR, another one, or I may do it later myself. 🙂
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Description
useOptions
Checklist