Vehicles #51
Vehicles #51
Conversation
… managing the setInterval/clearInterval w/in Component mount / unmount lifecyele routines
| retVal = true; | ||
| } | ||
| } | ||
| } catch (e) { |
There was a problem hiding this comment.
This try/catch shouldn't be needed at all.
| } catch (e) { | ||
| console.log(e); | ||
| } | ||
| return retVal; |
There was a problem hiding this comment.
This could be rewritten into a one-liner:
return !!vehicleA && !!vehicleB && (vehicleA.vehicleId === vehicleB.vehicleId || vehicleA.tripId === vehicleB.tripId);
| url = url.indexOf("?") ? `${url}?` : `${url}&`; | ||
| url = `${url}__time__=${d}`; | ||
|
|
||
| fetch(url) |
There was a problem hiding this comment.
Is there already a polyfill for this fetch statement in the webpack build? Not sure this will work in all browsers.
| */ | ||
| export function fetchVehicles(setData, trackedId, baseUrl, query) { | ||
| // build url | ||
| baseUrl = baseUrl || "https://maps.trimet.org/gtfs/rt/vehicles/"; |
There was a problem hiding this comment.
I think we should try to avoid TriMet-specific config and urls as much as possible. The component that calls this already has a default set, so there's probably no need to have a fallback value again here.
| fetch(url) | ||
| .then(res => { | ||
| if (!res.ok) { | ||
| console.log(res.statusText); |
There was a problem hiding this comment.
Please remove all console.log statements.
There was a problem hiding this comment.
This could be changed to console.error
| } | ||
| return res; | ||
| }) | ||
| .then(res => { |
There was a problem hiding this comment.
This then statement could be combined with the previous one.
| * fetch line geometry for a given pattern | ||
| * | ||
| * https://newplanner.trimet.org/ws/ti/v0/index/patterns/TRIMET:433758/geometry/geojson | ||
| * https://newplanner.trimet.org/ws/ti/v0/index/patterns/{agency}:{pattern}/geometry/geojson |
There was a problem hiding this comment.
Some more context might be able to be added to note that these URLs are examples to look at.
| tiUrl, | ||
| geojson = "/geojson" | ||
| ) { | ||
| if (!tiUrl) tiUrl = "https://newplanner.trimet.org/ws/ti/v0/index"; |
There was a problem hiding this comment.
Please remove default TriMet config
| } | ||
| }) | ||
| .catch(error => { | ||
| console.log(`VEH fetch() error: ${error} for ${url}`); |
There was a problem hiding this comment.
Should be console.error
| fetch(url) | ||
| .then(res => { | ||
| const retVal = res.json(); | ||
| return retVal; |
There was a problem hiding this comment.
Can be simply:
.then(res => res.json())
| return retVal; | ||
| }) | ||
| .then(json => { | ||
| if (geojson.indexOf("geojson") >= 0) { |
There was a problem hiding this comment.
Can you add a comment describing what this if statement is checking?
| setPatternData(patternId, json); | ||
| }) | ||
| .catch(error => { | ||
| console.log(`VEH GEOMETRY fetch() error: ${error}`); |
| return retVal; | ||
| } | ||
|
|
||
| export function formatTime(seconds) { |
There was a problem hiding this comment.
Could core-utils/src/time.js#formatDuration be modified to satisfy this use case?
| ); | ||
| } | ||
| } catch (e) { | ||
| console.log(e); |
| color: null, | ||
|
|
||
| geometryUrl: "https://newplanner.trimet.org/ws/ti/v0/index", | ||
| vehicleUrl: "https://maps.trimet.org/gtfs/rt/vehicles/", |
There was a problem hiding this comment.
Would prefer to not have TriMet-specific defaults.
| */ | ||
| function VehicleLayer(props) { | ||
| const { vehicles } = props; | ||
| const { trackedVehicle } = props; |
There was a problem hiding this comment.
Can be:
const { trackedVehicle, vehicles } = props;
| const { leaflet } = props; | ||
|
|
||
| const { closeZoom, midZoom } = props; | ||
| const { midSize, farSize } = props; |
There was a problem hiding this comment.
All these props can be destructured all at once.
| return icon; | ||
| } | ||
|
|
||
| const icon = makeIcon(); |
There was a problem hiding this comment.
It'd probably be a good idea to cache these icons, but that's probably something for a later effort.
| function VehiclePopup(props) { | ||
| const { vehicle } = props; | ||
| const { tracked } = props; | ||
| const { setTracked } = props; |
There was a problem hiding this comment.
Can all be destructed in one statement.
| vid = `Vehicle: ${vehicle.vehicleId}`; | ||
| } | ||
|
|
||
| const stopLink = `https://trimet.org/ride/stop.html?stop_id=${vehicle.stopId}`; |
There was a problem hiding this comment.
There should probably be a slot for a function to generate this link.
|
|
||
| return ( | ||
| <Popup> | ||
| <div> |
There was a problem hiding this comment.
Should be encapsulated within a styled component.
| function VehicleTracker(props) { | ||
| const { vehicle } = props; | ||
| const { tracked } = props; | ||
| const { setTracked } = props; |
There was a problem hiding this comment.
Props can be destructured all at once.
| const { tracked } = props; | ||
| const { setTracked } = props; | ||
|
|
||
| function handleClick() { |
There was a problem hiding this comment.
Seeing this function and the next makes me think the VehicleTracker should be converted into a Component. Otherwise, these functions are defined all over again on every render.
| const { trackedVehicle } = props; | ||
| const { pattern } = props; | ||
| const { lowlight, color } = props; | ||
| let { highlight } = props; |
There was a problem hiding this comment.
Can destructure all these props at once.
| highlight = setColor(color, highlight); | ||
| } | ||
|
|
||
| let retVal = <FeatureGroup />; |
There was a problem hiding this comment.
This could be moved to after the following if block and the FeatureGroup with the segments could be immediately returned.
| if (trackedVehicle && pattern && pattern.data) { | ||
| const pt = utils.findPointOnLine(trackedVehicle, pattern.data); | ||
| const geom = utils.splitGeometry(pattern.data, pt, pattern.id); | ||
| const segments = utils.makeSplitLine(geom, highlight, lowlight); |
There was a problem hiding this comment.
The calculations from the above lines probably can be cached somehow, but that might be something for a later effort.
Storybook is here: https://opentransittools.github.io/otp-ui/?path=/story/realtime-vehiclelayer--real-time-vehicles-layer
NOTE: storybook has 'knobs' that allows mucking with the interface.