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

Directions: Add marker on map + improve instruction panel #761

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

jvaclavik
Copy link
Collaborator

Description

Example links

Screenshots

image

Checklist

  • dark mode / light mode
  • mobile / desktop
  • server-side-rendering (SSR)

Copy link

vercel bot commented Nov 13, 2024

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

Name Status Preview Updated (UTC)
osmapp ✅ Ready (Inspect) Visit Preview Nov 13, 2024 5:57pm

@jvaclavik jvaclavik force-pushed the directions-add-marker-on-map branch 2 times, most recently from 3937787 to eb2d6fd Compare November 13, 2024 17:44
@jvaclavik jvaclavik force-pushed the directions-add-marker-on-map branch from eb2d6fd to 4a06b9f Compare November 13, 2024 17:45
@jvaclavik jvaclavik enabled auto-merge (squash) November 13, 2024 17:45
@jvaclavik jvaclavik disabled auto-merge November 13, 2024 17:47
@jvaclavik jvaclavik force-pushed the directions-add-marker-on-map branch from 4a06b9f to 72f62c6 Compare November 13, 2024 17:48
@jvaclavik jvaclavik force-pushed the directions-add-marker-on-map branch from 72f62c6 to 94b6485 Compare November 13, 2024 17:52
@jvaclavik jvaclavik force-pushed the directions-add-marker-on-map branch from 94b6485 to fda55fc Compare November 13, 2024 17:54
@jvaclavik jvaclavik merged commit d485236 into master Nov 13, 2024
3 checks passed
@jvaclavik jvaclavik deleted the directions-add-marker-on-map branch November 13, 2024 17:57
Comment on lines +193 to +200
const onDragEnd = () => {
const lngLat = markerRef.current?.getLngLat();
if (lngLat) {
setValue(getCoordsOption([lngLat.lng, lngLat.lat]));
}
};

markerRef.current?.on('dragend', onDragEnd);
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be inside the useEffect above. Currently this would add the listener on every component render, that can degrade the app dramatically.

@@ -146,6 +159,46 @@ export const DirectionsAutocomplete = ({ label, value, setValue }: Props) => {
const { userSettings } = useUserSettingsContext();
const { isImperial } = userSettings;

const ALPHABETICAL_MARKER = useMemo(() => {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you perhaps extract this functionality to a custom hook?


useEffect(() => {
const map = getGlobalMap();
if (value?.type === 'coords') {
Copy link
Owner

Choose a reason for hiding this comment

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

If the value is type 'geocoder', the marker isn't shown.

@zbycz
Copy link
Owner

zbycz commented Nov 14, 2024

btw, I love the new look of the Results panel. And also the green markers/circles are just beatiful! 🥇

@govvin
Copy link

govvin commented Nov 15, 2024

Thanks for your awesome work @jvaclavik ! ❤️

I observed that when a route has been generated, and a user decides to switch transportation mode, it doesn't automatically re-route, and the user needs to hit the "get directions" button again.

Don't you think it's more convenient for users to see the new route when they change the transportation mode?

@jvaclavik
Copy link
Collaborator Author

jvaclavik commented Nov 15, 2024

@govvin Thank you very much 🙏

We would like to improve it as you mentioned and I already created an issue for it: #763

It's blocked by using free tier in Graphhopper (max 500 requests per day), but I hope it's fixable 🤞

@Dlurak
Copy link
Collaborator

Dlurak commented Nov 15, 2024

It seems like this broke ssr. Opening this doesn't work but when it is generated on the client it works just fine

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.

4 participants