-
-
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
Public transport labels #175
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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, thanks for such a good PR! The code is nicely structured and readable. I would be very glad to merge it. Here are some questions:
return ( | ||
<div> | ||
{loading ? ( | ||
<div>...</div> |
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.
Could you please add "animated three dots loader" like here? It is much more clear for the user, when the loading is eg. stuck forever, etc.
Since this is shown only for Public transport entities, it could be perhaps shown even with the "Loading" string, or even better UX could be "Loading line numbers...", what do you think? When there is no line number, some text like "No lines number found for this station.", perhaps with link to overpass?
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.
I already wanted something like "Load routes..." but I don't know how internationalization is handled in this project.
setLoading(false); | ||
}; | ||
|
||
loadData(); |
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.
Would you be able to treat the error state please? If there is no connection or overpass throws "http error 500", it would leave the loading state forever.
You can take inspiration here, or perhaps add react-query library if you know it already.
export const PublicTransport: React.FC<PublicTransportProps> = ({ tags }) => { | ||
const isPublicTransport = | ||
Object.keys(tags).includes('public_transport') || | ||
tags.railway === 'station'; | ||
|
||
if (!isPublicTransport) { | ||
return null; | ||
} | ||
|
||
const [data, setData] = useState(null); |
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.
Could you please split this component after the condition? React forbids using hooks after an early return.
Just make another component PublicTransportInner
and put there everything from line 21 and so on.
const overpassQuery = `[out:csv(ref, colour; false; ';')]; | ||
${featureType}(${id})->.center; | ||
node(around.center:${radius})["public_transport"="stop_position"] -> .stops; | ||
rel(bn.stops)["route"~"bus|train|tram|subway|light_rail"]; | ||
out;`; |
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.
Can you please check why doesn't this work eg. here?
btw, is this query used somewhere else? i wonder if it is proofed by time to be correct.
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.
Some trains don't hold at every train station, so I search actively for holding positions in 150 meters radius around the selected feature. The problem was that I only searched for public_transport=stop_position
but this bus stop is tagged as highway=bus_stop
.
The query is not proven by time yet, I wrote it by myself just yesterday.
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.
I see, thanks for the fix and explanation 🚀
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.
Fails when two different stops are closer than 150 meters:
https://osmapp.org/node/3377755042
https://osmapp.org/node/3377755037
Maybe distinguish the stops by name?
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.
Good idea! But I am not sure if all stop positions have a name to them.
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.
I've implemented and tested this idea on my local machine, and it works well for stations where everything is mapped correctly, such as "Frankfurt am Main Hauptbahnhof." However, I've encountered a problem with stations that have different names for the stop positions compared to the train station itself. Here are some examples:
Train Station | Stop Positions |
---|---|
Friedberg (Hess) | Friedberg (Hessen) |
Brno hlavní nádraží | Brno hl.n. |
Wien Hauptbahnhof | Wien Hbf |
In Germany, central stations are generally referred to as "Hauptbahnhof" and often abbreviated as "Hbf." Some major train stations may have multiple levels with different names (e.g., "Berlin Hauptbahnhof" and "Berlin Hauptbahnhof (tief)"). However, I believe that separating their train lines might not be beneficial. There are also other cases where this approach isn't ideal.
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.
I like this conservative approach. If the Brno hlavní nádraží (Public transport) and Brno hl.n. (Trains) will be separated it is more acceptable than having trains on the different stops (Úzká). Even the completely opposite approach - lines only from this stop position I'd rather see than the faulty departures. But that's just my opinion!
Theoretically, the stops should be in a relation stop_area.
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.
I am not sure we are understanding each other correctly.
I created a new PR for the updates #180. What I mean is that when the node for the train station is called Brno hlavní nádraží but the name associated with the stop positions is Brno hl.n. the routes can't be clearly associated with the train station. So no routes will be shown, the vercel preview doesn't work but trust me it is the case. With the approach from #180 it will fail especially often for German central stations. Also it may become a problem with big bus stations.
The stop area relation seems interesting, do you know how well it is mapped in the real world?
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.
const g = parseInt(hexBgColor.slice(3, 5), 16); | ||
const b = parseInt(hexBgColor.slice(5, 7), 16); | ||
const brightness = (r * 299 + g * 587 + b * 114) / 1000; | ||
return brightness > 125 ? 'black' : 'white'; |
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.
very nice algorithm! 👍
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.
Works only when colour
is hex. But it can be also W3C colour name: https://wiki.openstreetmap.org/wiki/Key:colour#How_to_map
Fails for this yellow line 8: https://osmapp.org/node/3325029085
Maybe implement mapping name -> hex for the 16 "basic colour names"??
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.
Thanks for all the fixes, one little change and we are ready to merge. 🎉
const darkmode = window.matchMedia('(prefers-color-scheme: dark)').matches; | ||
|
||
let bgcolor: string; | ||
if (!color) bgcolor = darkmode ? '#898989' : '#dddddd'; |
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.
Colors are perfect. Could you please use the theme context? User can force light/dark if they wanted:
const { currentTheme } = useUserThemeContext();
if (currentTheme === 'dark') ...
Merged. Good job! Changes are already deployed to osmapp.org |
Overview
This PR partially fixes #168. When a public transport stop like an bus stop, train station, subway station or tram station gets clicked the user can now see which train, bus... lines hold at that stop.
Example