Add a new settings option for shuttle vehicle labels #192

arkadyan · 2019-09-11T19:23:44Z

Asana ticket: Add a new settings option for shuttle vehicle labels (vs. route ladder vehicle labels)

codecov · 2019-09-11T19:32:54Z

Codecov Report

Merging #192 into master will increase coverage by <.01%.
The diff coverage is 97.72%.

@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
+ Coverage   95.75%   95.76%   +<.01%     
==========================================
  Files         116      116              
  Lines        2452     2481      +29     
  Branches      255      266      +11     
==========================================
+ Hits         2348     2376      +28     
- Misses        102      103       +1     
  Partials        2        2

Impacted Files	Coverage Δ
assets/src/components/headwayDiagram.tsx	`100% <ø> (ø)`	⬆️
assets/src/components/vehiclePropertiesPanel.tsx	`95.37% <ø> (ø)`	⬆️
assets/src/components/layoverBox.tsx	`96.29% <ø> (ø)`	⬆️
assets/src/components/incomingBox.tsx	`100% <ø> (ø)`	⬆️
assets/src/components/ladder.tsx	`98.88% <ø> (ø)`	⬆️
assets/src/helpers/icon.tsx	`100% <100%> (ø)`	⬆️
assets/src/state.ts	`92.1% <100%> (+1.19%)`	⬆️
assets/src/components/tabBar.tsx	`100% <100%> (ø)`	⬆️
assets/src/helpers/vehicleLabel.ts	`100% <100%> (ø)`	⬆️
assets/src/components/map.tsx	`98.5% <100%> (ø)`	⬆️
... and 3 more

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 ac91de9...0119004. Read the comment docs.

skyqrose · 2019-09-11T20:49:19Z

assets/src/settings.ts

+  // special handling code in usePersistedStateReducer. -- MSS 2019-09-11
+  vehicleLabel: VehicleLabelSetting | undefined
+  ladderVehicleLabel: VehicleLabelSetting
+  mapVehicleLabel: VehicleLabelSetting


I think shuttleVehicleLabel might be a better name? We don't have this field because we draw some vehicles on a map. We have this field because shuttle have non-unique runs.

skyqrose · 2019-09-11T20:57:21Z

assets/src/helpers/icon.tsx

@@ -20,6 +22,34 @@ export const collapseIcon = (): JSX.Element => renderSvg("", collapseIconSvg)

 export const expandIcon = (): JSX.Element => renderSvg("", expandIconSvg)

+export const ladderIcon = (className: string = ""): JSX.Element => (


Would it be possible to set the .m-tab-bar__icon class on a parent element or to apply styles to a parent's CSS instead of needing to pass the class in, so that the icon can be a constant?

In the settings page, I think if the fill was put on .c-page__section-row-icon, then you wouldn't need to pass the .c-page__section-row-icon-path class in.

skyqrose · 2019-09-11T21:00:07Z

assets/src/helpers/icon.tsx

+      className={className}
+      d="m32.73 39.86v-12.08a4.37 4.37 0 0 1 0-7.56v-12.08a4.36 4.36 0 1 1 4.36 0v12.08a4.37 4.37 0 0 1 0 7.56v12.08a4.36 4.36 0 1 1 -4.36 0zm-17.46 0a4.36 4.36 0 1 1 -4.36 0v-12.08a4.37 4.37 0 0 1 0-7.56v-12.08a4.36 4.36 0 1 1 4.36 0v12.08a4.37 4.37 0 0 1 0 7.56z"
+    />
+  </svg>


Should the svgs be moved to separate .svg files to match the other icons loaded by this file?

skyqrose · 2019-09-11T21:02:02Z

assets/src/components/vehiclePropertiesPanel.tsx

@@ -100,7 +101,7 @@ const Header = ({
        <VehicleIcon
          size={Size.Large}
          orientation={Orientation.Up}
-          label={vehicleLabel(vehicle, settings.vehicleLabel)}
+          label={vehicleLabel(vehicle, vehicleLabelSetting(settings, vehicle))}


Right now the caller needs to make two calls, to vehicleLabelSettings and vehicleLabel. Could we get by with just one call to a vehicleLabel(vehicle, settings) that checks which setting to use?

Ideally, the setting to use would depend on the page you're on, not the vehicle you're looking at, but since you should only see a shuttle on the shuttle page and only see non-shuttles on the ladder, I think it'd be okay to always determine the label based on the vehicle?

arkadyan · 2019-09-12T18:43:09Z

@skyqrose Ready for another look, I think I've addressed everything.

skyqrose

LGTM

skyqrose · 2019-09-12T18:57:54Z

assets/static/images/icon-ladder.svg

+      d="m32.73 39.86v-12.08a4.37 4.37 0 0 1 0-7.56v-12.08a4.36 4.36 0 1 1 4.36 0v12.08a4.37 4.37 0 0 1 0 7.56v12.08a4.36 4.36 0 1 1 -4.36 0zm-17.46 0a4.36 4.36 0 1 1 -4.36 0v-12.08a4.37 4.37 0 0 1 0-7.56v-12.08a4.36 4.36 0 1 1 4.36 0v12.08a4.37 4.37 0 0 1 0 7.56z"
+    />
+  </svg>
+


The whitespace is a little weird in this file. There's a trailing line, and all lines except the 1st line (in both .svg files) are indented one extra space.

Oh yeah weird. Fixed thanks.

Render icons more like others.

arkadyan · 2019-09-12T19:56:31Z

Got a thumbs up from Ingrid.

reverts part of PR #192

arkadyan requested review from ingridpierre and skyqrose September 11, 2019 19:23

arkadyan assigned ingridpierre and skyqrose Sep 11, 2019

arkadyan force-pushed the mss-map-vehicle-label-setting branch 2 times, most recently from c2cf6be to ebf07ad Compare September 11, 2019 19:28

arkadyan force-pushed the mss-map-vehicle-label-setting branch from ebf07ad to 8c9a680 Compare September 11, 2019 20:09

skyqrose reviewed Sep 11, 2019

View reviewed changes

arkadyan force-pushed the mss-map-vehicle-label-setting branch from e01f511 to 41831b1 Compare September 12, 2019 18:42

skyqrose approved these changes Sep 12, 2019

View reviewed changes

arkadyan added 6 commits September 12, 2019 15:34

Add icon and “Route Ladders” in front of existing select box

b45d94d

Add a new setting for map view vehicle labels

ec40b06

Rename property to shuttleVehicleLabel

fc3bfc6

Have vehicleLabel function call vehicleLabelSetting function directly

86de7aa

Bring class names outside of svgs

1130412

Render icons more like others.

Fix whitespace

0119004

arkadyan force-pushed the mss-map-vehicle-label-setting branch from a3c53bc to 0119004 Compare September 12, 2019 19:35

arkadyan merged commit f2d4b36 into master Sep 12, 2019

arkadyan deleted the mss-map-vehicle-label-setting branch September 12, 2019 19:56

skyqrose added a commit that referenced this pull request Nov 26, 2019

remove old migration for settings property

54b320c

reverts part of PR #192

skyqrose mentioned this pull request Nov 26, 2019

remove old migration for settings property #334

Merged

skyqrose added a commit that referenced this pull request Dec 3, 2019

remove old migration for settings property (#334)

50a6e57

reverts part of PR #192

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a new settings option for shuttle vehicle labels #192

Add a new settings option for shuttle vehicle labels #192

arkadyan commented Sep 11, 2019

codecov bot commented Sep 11, 2019 •

edited

Loading

skyqrose Sep 11, 2019

skyqrose Sep 11, 2019

skyqrose Sep 11, 2019

skyqrose Sep 11, 2019

arkadyan commented Sep 12, 2019

skyqrose left a comment

skyqrose Sep 12, 2019

arkadyan Sep 12, 2019

arkadyan commented Sep 12, 2019

		@@ -20,6 +22,34 @@ export const collapseIcon = (): JSX.Element => renderSvg("", collapseIconSvg)

		export const expandIcon = (): JSX.Element => renderSvg("", expandIconSvg)

		export const ladderIcon = (className: string = ""): JSX.Element => (

Add a new settings option for shuttle vehicle labels #192

Add a new settings option for shuttle vehicle labels #192

Conversation

arkadyan commented Sep 11, 2019

codecov bot commented Sep 11, 2019 • edited Loading

Codecov Report

skyqrose Sep 11, 2019

Choose a reason for hiding this comment

skyqrose Sep 11, 2019

Choose a reason for hiding this comment

skyqrose Sep 11, 2019

Choose a reason for hiding this comment

skyqrose Sep 11, 2019

Choose a reason for hiding this comment

arkadyan commented Sep 12, 2019

skyqrose left a comment

Choose a reason for hiding this comment

skyqrose Sep 12, 2019

Choose a reason for hiding this comment

arkadyan Sep 12, 2019

Choose a reason for hiding this comment

arkadyan commented Sep 12, 2019

codecov bot commented Sep 11, 2019 •

edited

Loading