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

SearchBox: Refactoring and searchable stars #552

Merged
merged 7 commits into from
Sep 20, 2024

Conversation

Dlurak
Copy link
Collaborator

@Dlurak Dlurak commented Sep 17, 2024

Description

  • Add types to all of the searchbox options
  • Refactor useOptions
  • Make stars searchable
    • There was a todo comment to do this

Checklist

  • checked dark mode / light mode
  • checked mobile / desktop (Still only works with a double click)
  • checked server-side-rendering (SSR)
  • code style is consistent with the rest of the project
  • new dependencies are reasoned about in PR comments

Copy link

vercel bot commented Sep 17, 2024

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

Name Status Preview Updated (UTC)
osmapp ✅ Ready (Inspect) Visit Preview Sep 20, 2024 2:33pm

…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
zbycz
zbycz previously approved these changes Sep 20, 2024
Copy link
Owner

@zbycz zbycz left a 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.

src/components/SearchBox/AutocompleteInput.tsx Outdated Show resolved Hide resolved
src/components/SearchBox/SearchBox.tsx Outdated Show resolved Hide resolved
src/components/SearchBox/SearchBox.tsx Outdated Show resolved Hide resolved
.map((star) => ({
star,
// TODO matching is not optimal, maybe Sørensen–Dice coefficient
// https://www.npmjs.com/package/dice-coefficient
Copy link
Owner

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'][] };
Copy link
Owner

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. 🙂

src/components/SearchBox/onHighlightFactory.ts Outdated Show resolved Hide resolved
src/components/helpers.tsx Outdated Show resolved Hide resolved
@Dlurak Dlurak merged commit e01f753 into zbycz:master Sep 20, 2024
2 checks passed
Copy link

sentry-io bot commented Sep 24, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: Invalid LngLat object: (NaN, NaN) /[[...all]] View Issue
  • ‼️ TypeError: can't convert null to object /[[...all]] View Issue

Did you find this useful? React with a 👍 or 👎

@zbycz zbycz mentioned this pull request Sep 28, 2024
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