render route on map on table row hover #537

trxaphion · 2020-02-06T06:07:45Z

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/

exxonvaldez · 2020-02-07T02:11:11Z

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

trxaphion · 2020-02-10T09:55:13Z

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.

exxonvaldez · 2020-02-12T06:56:34Z

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.

trxaphion · 2020-02-12T16:09:10Z

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

Removed timeout on heroku demo.

hathix · 2020-02-13T03:15:50Z

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:

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.

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 · 2020-02-13T03:23:28Z

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

See feedback. Almost there!

hathix · 2020-02-13T03:24:18Z

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.

trxaphion · 2020-02-13T09:26:17Z

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.

exxonvaldez · 2020-02-26T02:33:13Z

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

This is looking good, I like the new styling for the highlighted route on the map. Also, don't know if you agree, but I like the code better with redux actions vs setTimeout. Seems like a nicer way to have components talk to each other that won't break if the component hierarchy gets rearranged.

I just had a few questions about some bits of the code, mostly from an ease of maintenance standpoint.

exxonvaldez · 2020-02-27T20:30:05Z

frontend/src/components/MapSpider.jsx

+  };
+
+  /**
+   * Get additional scale when near max zoom to account for increased periphery


I'm not sure I understand this comment, what is "increased periphery"? Also, I tried having this function always return 0 vs its current behavior at various zoom levels below and above 17. I couldn't actually see a difference, what is the issue this code solves? This method seems kind of hard to understand and maintain if we want to adjust how the map renders later.

It's really subtle. May just be me being nitpicky after staring at it for so long. Basically I find that when I'm zoomed out I'm looking near the marker so it's easy to spot when the route gets outlined but when I'm zoomed in to a hot spot with many routes on screen it's harder to notice, particularly when the route is less traveled and near the edge of the map.

Hmm, okay. I guess some change is needed here, either add the above text as a comment so others will better understand why this function is needed, or remove the function to keep things more maintainable and easier to understand.

Is this without the additional scale function? I’m fine with this, I’d rather not have additional logic for zoom levels greater than 17.

exxonvaldez · 2020-02-27T20:36:53Z

frontend/src/actions/index.js

+
+export function handleSegmentHover(segmentRouteId) {
+  return function(dispatch) {
+    dispatch({ type: 'RECEIVED_SEGMENT_HOVER', segmentRouteId });


The name "segment" threw me off for a bit, until I realized this was just the id of the route being hovered in the map. Can we change it to something like "spider hover", "map hover", or "spider map hover"?

Yeah, I think the term "segment" is going to take on a more specialized meaning now (as "the route between two adjacent stops"). Let's not use that term for other things.

Given Neel's description segment seems increasingly appropriate to me since "the route between two adjacent stops" is precisely what is being hovered. If anything shouldn't just the parameter be changed to routeId since the routeId is not specific to each segment?

Or we could throw all of that into a blender and have handleSpiderSegmentHover. I guess it really just depends how specific you want it.

Agree that segmentRouteId should just be routeId. Going by memory, I don’t remember the term “segment” being used in the front end code, so “handleSegmentHover” doesn’t mean much to me. handleSpiderSegmentHover is a lot better and is acceptable to me, but I’d prefer handleSpiderHover more.

exxonvaldez · 2020-02-27T20:43:31Z

frontend/src/components/MapSpider.jsx

+          <this.MemoizedDownstreamLines
+            latLng={spiderSelection.latLng}
+            stops={spiderSelection.stops}
+            statsByRouteId={statsByRouteId}


From what I understand of the React.memo docs, the idea is that the memoized component skips re-rendering if a shallow compare of the props matches the props used for rendering? So the props you're passing in are the ones that, if different, force rerendering of downstream lines? Can you add some comments to that effect? It's not intuitive at first, since DownstreamLines doesn't consume all these props directly.

exxonvaldez · 2020-02-27T20:45:46Z

frontend/src/components/RouteTable.jsx

+
+  useEffect(() => {
+    return () => onRowHover(null);
+  }, [onRowHover]);


What is the purpose of this useEffect? Looks like it only runs on first render? I tried commenting out and didn't see any difference.

I believe when I navigated to a different page while still having the route hovered (via back or forward on mouse) and then returning to the page the route is still hovered.

I see, was able to reproduce that. Can you add some comments to that effect? useEffect is a bit of swiss army knife, meaning it's hard to know what the effect is being used for. My interpretation is that this effect runs on mount and unmount to reset the table hover state.

exxonvaldez · 2020-02-27T21:00:57Z

frontend/src/components/RouteTable.jsx

@@ -414,14 +426,21 @@ function RouteTable(props) {
            rowCount={displayedRouteStats.length}
            columns={columns}
          />
-          <TableBody>
+          <TableBody onMouseLeave={() => onRowHover(null)}>
            {stableSort(displayedRouteStats, order, orderBy).map(row => {


This is sort of unrelated to your PR. We have this duplicate route id thing going on right now on the back end, and I noticed that it generates a bunch of console warnings due to duplicate TableRow keys. Also this breaks the spider map's filtering of the table rows. I noticed that filtering works again if we use unique keys -- so instead of keying on route id, we could just key on row index, like "routerow" + index.

Suggested change

{stableSort(displayedRouteStats, order, orderBy).map(row => {

{stableSort(displayedRouteStats, order, orderBy).map((row, index) => {

So are we just going to chuck it in here so we wont need a separate PR?

Well, the duplicate problem is fixed now, so I guess it’s not needed. It was mainly just to get the spider filtering working again to make testing this PR easier.

render route on map on table row hover

4fee553

trxaphion requested review from hathix, akgupta89 and exxonvaldez February 6, 2020 06:07

trxaphion requested a review from ckrajewski as a code owner February 6, 2020 06:07

trxaphion added 2 commits February 12, 2020 19:08

manage hovered line with redux

e2d3e13

undo line changes, white outline under route

3e88816

hathix requested changes Feb 13, 2020

View reviewed changes

trxaphion added 3 commits February 13, 2020 20:47

remove timeout

e15fb0c

highlight row on segment hover

f820498

thicker hover route outlines

bf1f988

trxaphion and others added 3 commits February 26, 2020 18:12

Merge branch 'master' into hover-line

4c76415

Merge branch 'master' into hover-line

785c05f

fix lint errors

37a4c52

exxonvaldez reviewed Feb 27, 2020

View reviewed changes

exxonvaldez mentioned this pull request Feb 28, 2020

Text labels for stops on MapStops component are obscured by markers of subsequent stops #573

Open

trxaphion added 4 commits March 1, 2020 22:21

change function and var names

e2fa526

remove additional scale on zoom

9727ea9

add comments

2a35ad2

make component output more readable

028d555

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

render route on map on table row hover #537

render route on map on table row hover #537

trxaphion commented Feb 6, 2020 •

edited

Loading

exxonvaldez commented Feb 7, 2020

trxaphion commented Feb 10, 2020 •

edited

Loading

exxonvaldez commented Feb 12, 2020

trxaphion commented Feb 12, 2020

hathix commented Feb 13, 2020

hathix commented Feb 13, 2020

hathix left a comment

hathix commented Feb 13, 2020

trxaphion commented Feb 13, 2020

exxonvaldez commented Feb 26, 2020

exxonvaldez left a comment

exxonvaldez Feb 27, 2020

trxaphion Feb 28, 2020

exxonvaldez Feb 28, 2020

trxaphion Feb 28, 2020

exxonvaldez Feb 28, 2020

exxonvaldez Feb 27, 2020

hathix Feb 28, 2020

trxaphion Feb 28, 2020 •

edited

Loading

trxaphion Feb 28, 2020

exxonvaldez Feb 28, 2020

exxonvaldez Feb 27, 2020

exxonvaldez Feb 27, 2020 •

edited

Loading

trxaphion Feb 28, 2020

exxonvaldez Feb 28, 2020

exxonvaldez Feb 27, 2020

trxaphion Feb 28, 2020

exxonvaldez Feb 28, 2020

	{stableSort(displayedRouteStats, order, orderBy).map(row => {
	{stableSort(displayedRouteStats, order, orderBy).map((row, index) => {

render route on map on table row hover #537

Are you sure you want to change the base?

render route on map on table row hover #537

Conversation

trxaphion commented Feb 6, 2020 • edited Loading

Proposed changes

Link to demo, if any

exxonvaldez commented Feb 7, 2020

trxaphion commented Feb 10, 2020 • edited Loading

exxonvaldez commented Feb 12, 2020

trxaphion commented Feb 12, 2020

hathix commented Feb 13, 2020

hathix commented Feb 13, 2020

hathix left a comment

Choose a reason for hiding this comment

hathix commented Feb 13, 2020

trxaphion commented Feb 13, 2020

exxonvaldez commented Feb 26, 2020

exxonvaldez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trxaphion Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exxonvaldez Feb 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trxaphion commented Feb 6, 2020 •

edited

Loading

trxaphion commented Feb 10, 2020 •

edited

Loading

trxaphion Feb 28, 2020 •

edited

Loading

exxonvaldez Feb 27, 2020 •

edited

Loading