metrics-mvp icon indicating copy to clipboard operation
metrics-mvp copied to clipboard

render route on map on table row hover

Open trxaphion opened this issue 5 years ago • 9 comments

Fixes #389

Proposed changes

Render route on map when hovering over route table. When no spider exists both directions are rendered. When spider exists only render directions already on the spider.

Currently on hover, the starting stop (closest stop to spider marker) of the route and stops are fully opaque whilst polylines are slightly less transparent (only when spider exists), and of course everything is slightly larger. I find that when the polylines are too transparent you sometimes see a mess of colors underneath making it harder to see but when it's too solid you can't see the stops. Thoughts?

Also the map shields of other routes currently stay on top of the hovered route (except the shield). Is this preferable?

Link to demo, if any

https://open-transit-hover-line.herokuapp.com/

trxaphion avatar Feb 06 '20 06:02 trxaphion

Thanks for doing this. This seems like a good enhancement overall, and it is very responsive (which I know can be a challenge with react-leaflet). I haven't used React.memo before but this seems like a natural place to use it.

I wanted to start with some high level feedback before getting into a detailed review. On the technical side, I was wondering if we can avoid the use of setTimeout/clearTimeout. These kinds of global function calls are difficult to understand and debug. Also, pushing state from Dashboard down to its children is a bit of an antipattern that Redux was meant to solve. Can you use Redux state to communicate between components and avoid the use of setTimeout? The spider selection state is done using Redux, you should be able to add stuff alongside that.

On the UI design side, everything a matter of subjective opinion but my two cents: I'd like the see the hover effect done a little differently. On MapStops, we use a wider white polyline to provide some contrast against the map, and then a wider indigo polyline between the selected stops. So can we do the hover effect using a white or indigo outline? I'd also prefer the shield sizes and colored line widths to stay as they were. The width conveys the frequency of the line, so that information gets lost when the width is scaled up, especially if there is no spider selection. I'm hoping the white or indigo outline lets you keep the opacities for the colored polylines where there were originally, and no changes to the shields.

It seems natural that the hover effect should go both ways, so when you highlight a route segment, the route's row in the table is also highlighted (the light gray background in the table row, which might be a css effect).

exxonvaldez avatar Feb 07 '20 02:02 exxonvaldez

Thanks for the detailed feedback.

My rationale behind the route only showing directions within range of the marker was that having a marker down already eliminated whole routes from the table making them unselectable, so the user would equate what is shown on the map to what remains on the table and vice versa. On the other hand clicking the route involves choosing either direction so if you still think it would be more natural to always show both directions then I don't disagree.

Regarding the timeout, removing it won't have a huge impact but it will be noticeable when moving through the routes consecutively at a somewhat fast rate.

noTimeout

trxaphion avatar Feb 10 '20 09:02 trxaphion

My rationale behind the route only showing directions within range of the marker was that having a marker down already eliminated whole routes from the table making them unselectable, so the user would equate what is shown on the map to what remains on the table and vice versa. On the other hand clicking the route involves choosing either direction so if you still think it would be more natural to always show both directions then I don't disagree.

I'm not sure what part of my feedback you're referring to here? It looks the visibility of routes when a selection is made hasn't changed in this PR, and that is good. Aside from stylistic changes to how the route highlights are done, I was suggesting that when the user mouses over the map (triggering the tooltip), we also highlight the route's row in the table.

Regarding the timeout, removing it won't have a huge impact but it will be noticeable when moving through the routes consecutively at a somewhat fast rate.

For this particular feature, I think that is fine. Let's try it out.

exxonvaldez avatar Feb 12 '20 06:02 exxonvaldez

My mistake, after I read the paragraph again it seems I misunderstood.

Removed timeout on heroku demo.

trxaphion avatar Feb 12 '20 16:02 trxaphion

Hey Raymond,

We were all going crazy about your feature at the meeting just now. In the words of one of our members, it's "tight."

Couple pieces of feedback:

Screen Shot 2020-02-12 at 7 10 12 PM

Terence was saying that the highlighting is very subtle now, making it hard to pick out the route. (Before it was a bit too much?) Perhaps something in between would be better.

Screen Shot 2020-02-12 at 7 10 28 PM

In the "live" version of OpenTransit, each line is a different width based on how often the bus comes. I don't see that in this version of yours. Can you put back that feature?

hathix avatar Feb 13 '20 03:02 hathix

I was suggesting that when the user mouses over the map (triggering the tooltip), we also highlight the route's row in the table.

Also a good suggestion from Terence. How hard would that be to add?

hathix avatar Feb 13 '20 03:02 hathix

Only change that's a "must" is putting the variable line widths back. Other things can be put in separate issues so we can address them later.

hathix avatar Feb 13 '20 03:02 hathix

I did already undo the changes to the scale, it just seems that the route stats didn't update beyond a point as you can see there are no scores on the route table. I didn't look into it as I had assumed it was from a conflict on the backend. If you go back to Feb 9 or earlier you will see there are stats and the lines reflect them.

As for highlighting the row it sounds possible.

trxaphion avatar Feb 13 '20 09:02 trxaphion

The code looks pretty good, but I can't test it effectively. I think there's some problems with the latest GTFS feed for Muni, from looking at some of the newest PRs, so we have duplicate routes.

When I click a point on the map, I still see most or all routes in the table, where I'm expecting to see just the routes that are visible on the map. I don't see a code change in this PR that would cause that, or maybe I missed it.

It might be helpful to address the git conflicts, as this should simplify the diffs a bit. A lot of changes are due to the linter/prettier fixing things, which are now also fixed on master.

exxonvaldez avatar Feb 26 '20 02:02 exxonvaldez