tweak: PreFare simulations #1837

cmaddox5 · 2023-08-24T19:22:55Z

Asana task: [Pre-Fare Alerts] "New look" in Screenplay

Now that there are two different "flex zones" on PreFare screens, a little tweaking was needed so users can see all pages on both screens. To make things simpler, PreFare has its own set of simulation components and CSS. A single server change was needed so grouping logic did not incorrectly group pages with the same page number of different screens.

Tests added?

github-actions · 2023-08-24T19:32:46Z

Coverage of commit `f2ca8ae`

Summary coverage rate:
  lines......: 45.8% (2760 of 6029 lines)
  functions..: 44.4% (1121 of 2526 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|55.2%    29|    -      0

Download coverage report

github-actions · 2023-08-24T19:38:05Z

Coverage of commit `f2ca8ae`

Summary coverage rate:
  lines......: 42.1% (2354 of 5592 lines)
  functions..: 42.5% (1034 of 2433 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|55.2%    29|    -      0

Download coverage report

jzimbel-mbta

Looks good! Just some thoughts and suggestions.

I do wonder how we would generalize the logic that divides paged content into its separate containers, but any approach that works for the narrow case of pre-fares is good enough for now.

assets/src/components/v2/pre_fare/simulation_screen_container.tsx

jzimbel-mbta · 2023-08-24T20:49:29Z

lib/screens/v2/screen_data.ex

-      fn {paged_slot_id, _} -> Template.get_page(paged_slot_id) end,
+      fn
+        {paged_slot_id, instance} ->
+          if Map.has_key?(instance, :is_full_screen) and instance.is_full_screen do


Not sure if it's clearer, but the [] access syntax returns nil for missing map keys:

iex> map = %{a: 1} iex> map[:q] nil iex> map.q ** (KeyError) key :q not found in: %{a: 1}

which, since nil is falsy, allows you do this check like

Suggested change

if Map.has_key?(instance, :is_full_screen) and instance.is_full_screen do

if instance[:is_full_screen] do

O yeah, I like that much better. Thanks!

jzimbel-mbta · 2023-08-24T21:01:45Z

lib/screens/v2/screen_data.ex

-      fn {paged_slot_id, _} -> Template.get_page(paged_slot_id) end,
+      fn
+        {paged_slot_id, instance} ->
+          if Map.has_key?(instance, :is_full_screen) and instance.is_full_screen do


Separately: it feels a little janky to distinguish paged stuff on the left screen from that in the right screen flex-zone, by a key that happens to be on the one type of widget that can currently occupy that space.

Is there any other way to distinguish them?

If I'm following the code correctly, paged_slot_id should look something like {0, :paged_main_content_left} for a widget occupying one of the paged left screen slots. Checking for that slot ID would be a bit better, since it's independent of whatever widget is going in the slot.

So maybe something like:

|> Enum.group_by( fn {{_n, :paged_main_content_left} -> :paged_main_content_left {paged_slot_id, _} -> Template.get_page(paged_slot_id) end, fn {paged_slot_id, instance} -> {Template.unpage(paged_slot_id), instance} end )

This still isn't ideal since it's specific to pre-fares, but maybe a bit better? 🤷

Yeah this is a tricky situation and definitely the what I spent the most time figuring out. This problem only exists because we get data for both screens at the same time (or if we ever have two flexzones on one screen for other apps). Because of this, I don't see any screen besides PreFare having this issue unless we add a flexzone to another screen.

Would it be cleaner to leverage app_id in this? Maybe bring that in as a parameter, refactor the key grouping anon function into a named function, and use a guard. What do you think?

I think yeah, it might be best to just have special behavior for the pre-fare app. A separate named function leaves room for defining clauses for for other apps later, if need be, and separates the app-specific stuff from the common logic a little bit better.

tl;dr Yes, a named paged_content_key (or some better name) function that also takes app ID as an argument, assuming that's easy to access from this function, sounds good.

Got that added. Mind taking one more look?

github-actions · 2023-08-25T12:28:13Z

Coverage of commit `88ded56`

Summary coverage rate:
  lines......: 42.1% (2354 of 5592 lines)
  functions..: 42.5% (1034 of 2433 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|55.2%    29|    -      0

Download coverage report

github-actions · 2023-08-28T12:43:28Z

Coverage of commit `feba683`

Summary coverage rate:
  lines......: 42.1% (2354 of 5592 lines)
  functions..: 42.5% (1034 of 2434 functions)
  branches...: no data found

Files changed coverage rate:
                                                                           |Lines       |Functions  |Branches    
  Filename                                                                 |Rate     Num|Rate    Num|Rate     Num
  ===============================================================================================================
  lib/screens/v2/screen_data.ex                                            |61.7%    133|53.3%    30|    -      0

Download coverage report

jzimbel-mbta

Double approved ✅✅

* feat: Pre-Fare alerts 2.0 CandidateGenerator (#1765) * [feature] Pre-fare alert 2.0 frontend left screen (#1787) * feat: Pre-Fare alerts 2.0 serializer (#1785) * feat: Disruption Diagram - Frontend (#1825) * [tweak] Disruption diagram maths (#1830) * In LocationContext, tag stop sequences by their routes (#1832) * tweak: PreFare simulations (#1837) * [feature] Prefare alerts 2.0 audio (#1824) * feat: Disruption Diagram - Backend (#1828) * feat(WIP): Disruption diagram backend. Working for Blue Line and other basic scenarios only atm. * fix: Update location context stop sequences field in TrainCrowdingTest (#1895) * test: D. diagram unit test for RL trunk statio closure with home stop at Ashmont (#1894) * Added padding-top to layout when there is no banner. (#1902) * Cm/govt ctr gl affected pill fix (#1898) * Cm/adjust diagram size positioning (#1896) * Changed label to display on a single line. (#1899) * Tweaked resizer logic so it maxHeight matches the card height on page. (#1905) * Removed bolding of 'to' from arrow labels. (#1908) * Fixed icon display when current stop is affected. (#1903) * feat: Consistent description of disrupted stop ranges throughout pre-fare alerts (#1912) * Adjusted Ashmont/Braintree destination to proper formatting (#1917) * Refactor disruption diagram SVG (#1914) * Log alert ID and screen's home stop when disruption diagram logic fails (#1920) * polish: Recalculate dimensions in hook (#1921) * Added audio column to GL & PreFare in admin table (#1916) * Draw D. diagrams for multi-line alerts when home stop narrows it to 1 line (#1922) * fix: PreFare Polish - Inside shuttle (#1941) * fix: Pre-fare polish - Boundary alerts banner (#1945) * fix: Pre-fare polish -- Text only resizes for bypassed stations (#1946) * fix: Pre-fare polish -- Station closed takeover text wrapping (#1947) * Downstream shuttle endpoint should be a circle (#1948) * Label splitting logic was buggy (#1949) * Prefare format in config is "Ashmont & Braintree" not "Ashmont/Braintree" (#1954) * Sub-header for takeover alert is now regular weight (#1955) * fix: Prefare alerts polish -- Fix GL headsign in various cases (#1957) * fix: Prefare polish -- Use FreeText to prevent station text wrapping in subheaders (#1956) * fix: Prefare polish -- Alerts that break assumptions use the fallback case (#1958) * fix: Prefare polish -- Reorder audio with alerts before subway status (#1962) * fix: Prefare polish -- New abbreviation rule (#1961) * fix: Prefare Alerts polish -- RL headsigns for branch-only alerts (#1967) * fix: Prefare alerts polish -- Screenplay sim sizing / page layout (#1966) * fix: Prefare polish -- "via" diagram labels need abbreviating (#1968) --------- Co-authored-by: Christian Maddox <[email protected]> Co-authored-by: Jon Zimbel <[email protected]>

cmaddox5 added 3 commits August 24, 2023 15:20

Added new simulation component for PreFare.

1dd3090

Fixed CSS according to new designs.

078362b

Fixed grouping for single screen alerts.

f2ca8ae

cmaddox5 requested a review from jzimbel-mbta August 24, 2023 19:22

cmaddox5 assigned jzimbel-mbta Aug 24, 2023

cmaddox5 mentioned this pull request Aug 24, 2023

tweak: PreFare iframe container height. mbta/screenplay#221

Merged

jzimbel-mbta approved these changes Aug 24, 2023

View reviewed changes

jzimbel-mbta assigned cmaddox5 and unassigned jzimbel-mbta Aug 24, 2023

Removed unused prop.

88ded56

Added a named function for flexzone page grouping keys.

feba683

cmaddox5 requested a review from jzimbel-mbta August 28, 2023 12:36

cmaddox5 assigned jzimbel-mbta and unassigned cmaddox5 Aug 28, 2023

jzimbel-mbta approved these changes Aug 30, 2023

View reviewed changes

jzimbel-mbta assigned cmaddox5 and unassigned jzimbel-mbta Aug 30, 2023

cmaddox5 merged commit d7e9352 into prefare-alerts Aug 30, 2023

cmaddox5 deleted the cm/single-screen-alert-simulation branch August 30, 2023 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tweak: PreFare simulations #1837

tweak: PreFare simulations #1837

cmaddox5 commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

jzimbel-mbta left a comment

jzimbel-mbta Aug 24, 2023

cmaddox5 Aug 25, 2023

jzimbel-mbta Aug 24, 2023

cmaddox5 Aug 25, 2023

jzimbel-mbta Aug 25, 2023

jzimbel-mbta Aug 25, 2023

cmaddox5 Aug 28, 2023

github-actions bot commented Aug 25, 2023

github-actions bot commented Aug 28, 2023

jzimbel-mbta left a comment

	if Map.has_key?(instance, :is_full_screen) and instance.is_full_screen do
	if instance[:is_full_screen] do

tweak: PreFare simulations #1837

tweak: PreFare simulations #1837

Conversation

cmaddox5 commented Aug 24, 2023

github-actions bot commented Aug 24, 2023

Coverage of commit f2ca8ae

github-actions bot commented Aug 24, 2023

Coverage of commit f2ca8ae

jzimbel-mbta left a comment

Choose a reason for hiding this comment

jzimbel-mbta Aug 24, 2023

Choose a reason for hiding this comment

cmaddox5 Aug 25, 2023

Choose a reason for hiding this comment

jzimbel-mbta Aug 24, 2023

Choose a reason for hiding this comment

cmaddox5 Aug 25, 2023

Choose a reason for hiding this comment

jzimbel-mbta Aug 25, 2023

Choose a reason for hiding this comment

jzimbel-mbta Aug 25, 2023

Choose a reason for hiding this comment

cmaddox5 Aug 28, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 25, 2023

Coverage of commit 88ded56

github-actions bot commented Aug 28, 2023

Coverage of commit feba683

jzimbel-mbta left a comment

Choose a reason for hiding this comment

Coverage of commit `f2ca8ae`

Coverage of commit `f2ca8ae`

Coverage of commit `88ded56`

Coverage of commit `feba683`