Add tooltip for timepoints #452

arkadyan · 2020-02-13T18:45:45Z

Asana ticket: Route ladders polish | add timepoint tool tip

codecov · 2020-02-13T18:50:32Z

Codecov Report

Merging #452 into master will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #452   +/-   ##
=======================================
  Coverage   97.36%   97.36%           
=======================================
  Files         153      154    +1     
  Lines        3527     3532    +5     
  Branches      461      461           
=======================================
+ Hits         3434     3439    +5     
  Misses         90       90           
  Partials        3        3

Impacted Files	Coverage Δ
lib/gtfs/timepoint.ex	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa57c03...28a7efc. Read the comment docs.

skyqrose

Not finished reviewing, I'll continue tomorrow, but there's some comments that I didn't want to hold up til I finished.

skyqrose · 2020-02-13T22:07:44Z

lib/gtfs/data.ex

-  @spec timepoint_ids_for_routes([RoutePattern.t()], MapSet.t(Route.id()), trips_by_id()) ::
-          timepoint_ids_by_route()
-  defp timepoint_ids_for_routes(route_patterns, route_ids, trips) do
+  @spec all_timepoints_by_id(binary()) :: timepoints_by_id()


Could/should this function (and/or the timepoints_by_id type) go in timepoint.ex instead?

I don't have strong feelings about this. This is mirroring many other equivalent functions in this module. I thought in the past you had expressed the opinion that these should be here instead of in e.g. Timepoint, but my memory is vague so I could easily be getting that wrong.

lib/gtfs/data.ex

skyqrose · 2020-02-13T22:38:33Z

lib/gtfs/stop_time.ex


  @type t :: %__MODULE__{
          stop_id: Stop.id(),
          time: Util.Time.time_of_day(),
-          timepoint_id: timepoint_id() | nil
+          timepoint: Timepoint.t() | nil


I think stop_time should still have a Timepoint.id(). Then in the rare case we need the full timepoint (I think just for getting the list of full timepoints for a route), we look up the ids in a separate table that forms a single source of truth. For most things we do with timepoint ids (e.g. measuring a bus's progress), having an id reference would be slimmer (important since all the stop_times really bog us down) and easier to work with when doing equality checks.

What do you mean by "slimmer" and "bog us down"? I'm not sure I understand the problem you are trying to solve, but adding a store for these and an additional lookup for every vehicle on every update seems pretty expensive when this data is only used as a part of stop times.

The lookup wouldn't happen on every update. The vehicles don't need to know the timepoint names. We only need it for routes. So, once on startup, we would get the %{Route.id() => [Timepoint.id()]} map as we do on master, then look those up into a %{Route.id() => [Timepoint.t()]} map.

I guess, after we store the timepoints_by_route, we might not even need to save the full timepoints with names anywhere else.

assets/css/_ladder.scss

skyqrose · 2020-02-13T22:50:18Z

assets/src/components/ladder.tsx

          )
        })}
      </svg>
+      <ReactTooltip />


Does this only have to be included once, or does it need to be near every place where tooltips appear? If we only need it once, it should probably go on <LadderPage> (if we only want it when tooltips are on the screen) or App (if it should always be there) instead of on every ladder.

This is a good thought, but I just tried it and it does need to be in the same component as it's referenced in.

ingridpierre · 2020-02-13T22:59:54Z

Copied from Slack:
Regarding tooltip, we shouldn’t trigger the tooltip on hover, just click. Sorry if that wasn’t clear from me.

That’s not always the normal behavior for a tooltip, but it makes the experience more similar on desktop vs tablet and makes it overall less annoying as you scan around with the cursor.

lib/gtfs/stop_time.ex

lib/gtfs/data.ex

lib/realtime/timepoint_status.ex

arkadyan · 2020-02-14T16:06:18Z

Is it possible that it drops any timepoints that aren’t in checkpoints.txt?

arkadyan · 2020-02-14T17:48:41Z

@ingridpierre I updated it to only pop up the tooltip on click/tap, not on hover. I also set it so that the tooltip pops up in the middle of the timepoint each time, not where you happen to tap on it. The updates are up on dev now, let me know if this behaves more like you were looking for.

arkadyan · 2020-02-14T18:26:12Z

Added a fix to accommodate timepoints with no name. They now appear correctly on the ladder, but no tooltip appears if you try to tap on it.

I reported this case to the realtime team.

ingridpierre · 2020-02-18T20:29:25Z

Mildly funky bugs I experienced:

If I click a tooltip, then click to add another route from the picker, the opened tooltip doesnt go away and I can have multiple open at once (image below)
If I click a routes closed, I can no longer open up tooltips on the remaining ladders.

ingridpierre · 2020-02-18T20:33:29Z

Another small detail - for our desktop users I think it would be kind to show the cursor style change (like hovering over a link), so that you can know where to click down for the tooltip. At the moment I see a text highlight cursor on hover which is not as helpful.

arkadyan · 2020-02-18T20:40:06Z

@ingridpierre Changed the cursor to a pointer.

arkadyan · 2020-02-19T16:10:51Z

@ingridpierre I changed the behavior such that when you tap anywhere the tooltip closes (not just on the timepoint name. I think this is probably better behavior anyway and fixes the first bug around opening routes.

Unfortunately, it does not fix the second bug as I was hoping it would. How bad do you feel that bug is? If we wrote up a bug ticket and released this as is, do you think that would be ok, or is this issue too detrimental to the experience? At this point I don't have any ideas for how to solve this bug, so I think the continued work could potentially be a lengthy investigation and reworking that could easily take longer than what we've done so far.

ingridpierre · 2020-02-19T16:22:14Z

@arkadyan I don't think folks are closing ladders with a terrible frequency. Thankfully with the cookie people don't really have to manage the routes in the picker too often.

But when you do encounter it, it's pretty befuddling. I'd definitely still classify it as a gross bug and set it off for later.

Will take a look at the new changes to make sure I haven't misspoken.

ingridpierre · 2020-02-19T16:30:10Z

@arkadyan lgtm 👍

arkadyan · 2020-02-19T16:35:11Z

Recorded the bug here: https://app.asana.com/0/1148853526253420/1162700422074841. It is most certainly gross and befuddling

arkadyan · 2020-02-24T18:19:10Z

@skyqrose Moved the timepoint assignments to be by route pattern.

assets/src/schedule.d.ts

lib/gtfs/data.ex

lib/gtfs.ex

skyqrose · 2020-02-24T21:06:36Z

assets/src/components/ladder.tsx

@@ -220,23 +218,25 @@ const LadderTimepoint = ({
      y={y}
      textAnchor="middle"
      dominantBaseline="middle"
+      data-tip={timepoint.name}


If the name is missing, this would be null. Does react tooltip handle that well?

(Not relevant if we decide this comment is a good idea: #452 (comment) )

Yes, you can see this for "CENJF" on route 502: there's no tooltip if there's no content.

skyqrose · 2020-02-24T21:09:50Z

lib/gtfs/timepoint.ex

+  def timepoint_for_id(_timepoints_by_id, nil), do: nil
+
+  def timepoint_for_id(timepoints_by_id, id) do
+    Map.get(timepoints_by_id, id, %__MODULE__{id: id})


Instead of my other comments about the frontend handling a missing name, what do you think about creating a default %__MODULE__{id: id, name: id}, where the id is used as the name for the couple timepoints that we're missing, so we can depend on the name always being present?

At least for now we've said that we don't want to display a tooltip if there is no name.

lib/gtfs/timepoint.ex

test/gtfs_test.exs

Pass full timepoint data to the client-side.

Require the react-tooltip library and types.

It no longer follows your cursor or tap position.

arkadyan requested review from ingridpierre and skyqrose February 13, 2020 18:45

arkadyan assigned ingridpierre and skyqrose Feb 13, 2020

skyqrose reviewed Feb 13, 2020

View reviewed changes

skyqrose reviewed Feb 14, 2020

View reviewed changes

lib/gtfs/stop_time.ex Outdated Show resolved Hide resolved

lib/gtfs/data.ex Show resolved Hide resolved

lib/realtime/timepoint_status.ex Outdated Show resolved Hide resolved

arkadyan force-pushed the mss-timepoint-tool-tip branch from d7877c8 to 55e4c0f Compare February 14, 2020 17:26

arkadyan force-pushed the mss-timepoint-tool-tip branch from 3700609 to 164c5ab Compare February 18, 2020 15:15

arkadyan force-pushed the mss-timepoint-tool-tip branch from c6a8567 to be9335f Compare February 19, 2020 16:01

arkadyan force-pushed the mss-timepoint-tool-tip branch 4 times, most recently from 472161c to b8930e6 Compare February 24, 2020 18:18

arkadyan force-pushed the mss-timepoint-tool-tip branch from b8930e6 to ca75f09 Compare February 24, 2020 19:32

skyqrose reviewed Feb 24, 2020

View reviewed changes

arkadyan added 2 commits February 25, 2020 14:56

Add a test for StopTime.is_timepoint?/1

5bd5e53

Read in timepoint data that includes the names

92e9da5

Pass full timepoint data to the client-side.

arkadyan added 7 commits February 25, 2020 14:56

Add tooltip to timepoints

10bd152

Require the react-tooltip library and types.

Make the tooltip always appear in the center of the timepoint

4de90de

It no longer follows your cursor or tap position.

Only show the tooltip on click, not hover

49f2a69

Use a pointer cursor for the timepoint names

9e31667

Move the css rule to app

3166eb6

Hide the tooltip when you tap anywhere

561531d

Save timepoints by route patterns

a77ffc8

arkadyan force-pushed the mss-timepoint-tool-tip branch from ca75f09 to a77ffc8 Compare February 25, 2020 19:56

arkadyan added 5 commits February 25, 2020 15:03

Timepoint name can be null

ea7de89

Use Map.new

b9fadde

Fix return type

66010c8

Filter before getting timepoints

2a7b54e

Remove unneeded test data

28a7efc

arkadyan requested a review from skyqrose February 25, 2020 20:19

skyqrose approved these changes Feb 25, 2020

View reviewed changes

arkadyan merged commit 33d190a into master Feb 25, 2020

arkadyan deleted the mss-timepoint-tool-tip branch February 25, 2020 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tooltip for timepoints #452

Add tooltip for timepoints #452

arkadyan commented Feb 13, 2020

codecov bot commented Feb 13, 2020 •

edited

Loading

skyqrose left a comment

skyqrose Feb 13, 2020

arkadyan Feb 14, 2020

skyqrose Feb 13, 2020

arkadyan Feb 14, 2020

skyqrose Feb 14, 2020

skyqrose Feb 13, 2020

arkadyan Feb 14, 2020

ingridpierre commented Feb 13, 2020

arkadyan commented Feb 14, 2020

arkadyan commented Feb 14, 2020

arkadyan commented Feb 14, 2020

ingridpierre commented Feb 18, 2020 •

edited

Loading

ingridpierre commented Feb 18, 2020

arkadyan commented Feb 18, 2020

arkadyan commented Feb 19, 2020

ingridpierre commented Feb 19, 2020

ingridpierre commented Feb 19, 2020

arkadyan commented Feb 19, 2020

arkadyan commented Feb 24, 2020

skyqrose Feb 24, 2020 •

edited

Loading

arkadyan Feb 25, 2020

skyqrose Feb 24, 2020

arkadyan Feb 25, 2020

Add tooltip for timepoints #452

Add tooltip for timepoints #452

Conversation

arkadyan commented Feb 13, 2020

codecov bot commented Feb 13, 2020 • edited Loading

Codecov Report

skyqrose 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

ingridpierre commented Feb 13, 2020

arkadyan commented Feb 14, 2020

arkadyan commented Feb 14, 2020

arkadyan commented Feb 14, 2020

ingridpierre commented Feb 18, 2020 • edited Loading

ingridpierre commented Feb 18, 2020

arkadyan commented Feb 18, 2020

arkadyan commented Feb 19, 2020

ingridpierre commented Feb 19, 2020

ingridpierre commented Feb 19, 2020

arkadyan commented Feb 19, 2020

arkadyan commented Feb 24, 2020

skyqrose Feb 24, 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

codecov bot commented Feb 13, 2020 •

edited

Loading

ingridpierre commented Feb 18, 2020 •

edited

Loading

skyqrose Feb 24, 2020 •

edited

Loading