Mobile batch results #339

binh-dam-ibigroup · 2021-03-05T19:35:41Z

This PR displays the list of itinerary options in mobile views when batch routing is enabled.

This PR builds upon #337. Behind the scenes, the code that renders headers and error screen is shared between the existing and new mobile result view and extracted into ~~its~~ their own components.

Also: A new ui_itineraryView URL parameter is added to reflect the itinerary expanded state, right now it is available only for mobile batch results.

EDIT: Also fix #348.

Screenshot:

binh-dam-ibigroup · 2021-03-05T19:56:08Z

I marked this PR as blocked pending #337.

evansiroky · 2021-03-11T06:42:56Z

example.js

@@ -71,6 +73,9 @@ const components = {
      ? BatchRoutingPanel
      : DefaultMainPanel,
  MapWindows: isCallTakerModuleEnabled ? CallTakerWindows : null,
+  MobileResultsScreen: isBatchRoutingEnabled


Add this component as a required component in the comment about the creation of the components variable.

Updated in e802be6.

evansiroky

Approved pending resolution of 1 comment

landonreed

I think a few things might need some work, but this is headed in the right direction!

lib/components/mobile/batch-results-screen.js

landonreed · 2021-03-22T14:48:31Z

lib/components/mobile/batch-results-screen.js

+  constructor () {
+    super()
+    this.state = {
+      itineraryExpanded: false,


I'm not sure if it should be in this PR, but I'd like this itin expanded item to perhaps be moved to a UI query param. Might require some changes to the desktop view as well.

I went ahead and added a URL parameter ui_itineraryView in 3edc3d4 to replace passing props around to track the map expanded state.

lib/components/mobile/batch-results-screen.js

landonreed · 2021-03-22T14:53:27Z

lib/components/mobile/batch-results-screen.js

+          // Set up two separate renderings of the map according to mapExpanded,
+          // so that the map is properly sized and itineraries fit under either conditions.
+          // (Otherwise, if just the narrative is added/removed, the map doesn't resize properly.)
+          ? this.renderMap()


Is it possible to have this transition be animated (transition animation to expand the map div and possibly remove the refresh on the map tiles and have a smoother zoom to itinerary)? I find the current approach kind of jarring.

If we don't rerender the map and simply resize it (what we'd have to do to enable the animated transition), then the inner map canvas is not properly resized... any suggestions for triggering that?

I think you would have to reload the tiles anyway after expanding/collapsing the map because the zoom and fit would be different in either case.

Okay, I found a way to do the transition without rerendering the map with 557bfc7 (after almost giving up... - see the transitive trick in 2e7757c)

lib/components/mobile/batch-results-screen.js

lib/components/mobile/results-header-and-error.js

…eg as a prop to ItineraryBody c Both files were not passing setActiveLeg as a prop so I imported where necessary and passed in the prop. fix #338

The previous commit had a line of unused code. 338

codecov-io · 2021-03-23T16:11:03Z

Codecov Report

Merging #339 (7a8b67e) into dev (0457a25) will decrease coverage by 1.68%.
The diff coverage is 7.73%.

@@           Coverage Diff            @@
##             dev    #339      +/-   ##
========================================
- Coverage   9.78%   8.09%   -1.69%     
========================================
  Files        115     193      +78     
  Lines       4078    7201    +3123     
  Branches    1077    1807     +730     
========================================
+ Hits         399     583     +184     
- Misses      3191    5712    +2521     
- Partials     488     906     +418

Impacted Files	Coverage Δ
lib/actions/auth.js	`0.00% <0.00%> (ø)`
lib/actions/call-taker.js	`0.00% <0.00%> (ø)`
lib/actions/field-trip.js	`0.00% <0.00%> (ø)`
lib/actions/form.js	`22.22% <0.00%> (-0.36%)`	⬇️
lib/actions/user.js	`0.00% <0.00%> (ø)`
lib/components/admin/call-record.js	`0.00% <0.00%> (ø)`
lib/components/admin/call-taker-controls.js	`0.00% <0.00%> (ø)`
lib/components/admin/call-taker-windows.js	`0.00% <0.00%> (ø)`
lib/components/admin/call-time-counter.js	`0.00% <0.00%> (ø)`
lib/components/admin/draggable-window.js	`0.00% <0.00%> (ø)`
... and 293 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 b0760d4...7a8b67e. Read the comment docs.

landonreed · 2021-04-09T15:14:14Z

@binh-dam-ibigroup I'm not sure if this is from recent changes, but when viewing a leg on the map and then clicking "Expand map" it momentarily flashes the list of options:

Screen.Recording.2021-04-09.at.11.12.03.AM.mov

binh-dam-ibigroup · 2021-04-09T19:23:14Z

@binh-dam-ibigroup I'm not sure if this is from recent changes, but when viewing a leg on the map and then clicking "Expand map" it momentarily flashes the list of options:

Screen.Recording.2021-04-09.at.11.12.03.AM.mov

Ugh side effect. Fixed in af9f653.

landonreed · 2021-04-09T20:32:14Z

lib/components/narrative/narrative-itineraries.js

    activeStep: activeSearch && activeSearch.activeStep,
+    errors: getResponsesWithErrors(state.otp),
+    filter: state.otp.filter,


I'm not sure if this is needed any longer?

Thanks, updated in 3f138bb.

landonreed

Looks pretty good! Just one comment on the filter prop. Let's wait for @evansiroky's review to merge on Monday.

…nner/otp-react-redux into mobile-batch-results

evansiroky · 2021-04-12T20:05:28Z

lib/components/mobile/batch-results-screen.js

+class BatchMobileResultsScreen extends Component {
+  state = {
+    // Holds the previous split view state
+    previousSplitView: null


Since this state property is always used in conjunction with a prop from the overall store, it might be cleaner to move this into the overall application state and also make the toggleMapExpanded handler be an action. That way, this component can become a purely presentational component. This recommendation is non-blocking.

Thanks, this simplies the UI component a good bit (43fef77).

evansiroky · 2021-04-12T20:20:33Z

lib/components/mobile/edit-search-button.js

+  _handleClick = () => {
+    const { clearActiveSearch, setItineraryView, setMobileScreen } = this.props
+
+    // Reset itinerary view state to show the list of results *before* clearing the search.
+    // (Otherwise, if the map is expanded, the search is not cleared.)
+    setItineraryView(uiActions.ItineraryView.LIST)
+    clearActiveSearch()
+    setMobileScreen(uiActions.MobileScreens.SEARCH_FORM)
+  }


It would simplify this component a fair amount if this method were converted into a connected action. This comment in non-blocking.

Thanks for the suggestion, updated in 43fef77.

evansiroky

Just two non-blocking recommendations for improvement.

Fix #348

github-actions · 2021-04-13T18:04:27Z

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

binh-dam-ibigroup added 6 commits March 3, 2021 16:40

feat(mobile/batch-results-screen): Add mobile batch results screen.

ec0662d

refactor(mobile/BatchResultsScreen): Clean up code.

d35ce7f

refactor(BatchResultsScreen): Fix fitting itinerary after expanding map.

b1a84b6

refactor(BatchResultsScreen): Tweak styles.

9c21e13

refactor(mobile/ResultsHeaderAndError): Extract component. Refactors.

90be15a

style(mobile/*results*): Tweak comments and sort props and imports.

50d8d23

binh-dam-ibigroup marked this pull request as ready for review March 5, 2021 19:50

binh-dam-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Mar 5, 2021

binh-dam-ibigroup assigned landonreed and evansiroky Mar 5, 2021

binh-dam-ibigroup requested review from evansiroky and landonreed March 5, 2021 19:56

Base automatically changed from responsive-webapp-refactor to batch-routing-mobile-search March 10, 2021 21:10

Base automatically changed from batch-routing-mobile-search to dev March 10, 2021 21:12

landonreed removed the BLOCKED Blocked (waiting on another PR to be merged) label Mar 10, 2021

evansiroky reviewed Mar 11, 2021

View reviewed changes

evansiroky approved these changes Mar 11, 2021

View reviewed changes

evansiroky assigned binh-dam-ibigroup and unassigned evansiroky Mar 11, 2021

refactor(batch results): Mention component is required, fix typo.

e802be6

binh-dam-ibigroup removed their assignment Mar 12, 2021

landonreed requested changes Mar 22, 2021

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Mar 22, 2021

Rob Gregg and others added 3 commits March 23, 2021 11:50

fix(narrative-itineraries.js, default-itinerary.js): Added setActiveL…

c10d7ca

…eg as a prop to ItineraryBody c Both files were not passing setActiveLeg as a prop so I imported where necessary and passed in the prop. fix #338

fix(default-itinerary.js): Removed some unused code.

edea628

The previous commit had a line of unused code. 338

fix(BatchResultsScreen): Fix leg focus, address some PR comments.

100c1d6

binh-dam-ibigroup mentioned this pull request Mar 23, 2021

Walk-only, bike-only results returned in excess of max walk/bike distance. #347

Closed

binh-dam-ibigroup requested a review from evansiroky April 5, 2021 16:15

binh-dam-ibigroup assigned evansiroky and unassigned binh-dam-ibigroup Apr 5, 2021

landonreed assigned binh-dam-ibigroup and evansiroky and unassigned evansiroky and landonreed Apr 9, 2021

binh-dam-ibigroup added 2 commits April 9, 2021 15:10

fix(actions/ui): Split ItineraryView hidden state.

af9f653

Merge branch 'dev' into mobile-batch-results

403ca61

binh-dam-ibigroup assigned landonreed and unassigned binh-dam-ibigroup Apr 9, 2021

refactor(state.js): remove noisy/unneeded log statement

7a8b67e

landonreed reviewed Apr 9, 2021

View reviewed changes

landonreed approved these changes Apr 9, 2021

View reviewed changes

landonreed removed their assignment Apr 9, 2021

binh-dam-ibigroup added 2 commits April 12, 2021 09:36

refactor(NarrativeItineraries): Remove unused prop.

3f138bb

Merge branch 'mobile-batch-results' of https://github.com/opentrippla…

602e2ff

…nner/otp-react-redux into mobile-batch-results

evansiroky reviewed Apr 12, 2021

View reviewed changes

evansiroky approved these changes Apr 12, 2021

View reviewed changes

evansiroky assigned binh-dam-ibigroup and unassigned evansiroky Apr 12, 2021

binh-dam-ibigroup added 2 commits April 13, 2021 11:39

refactor(actions/ui): Extract batch results and edit search actions.

43fef77

fix(mobile.css): Make batch settings appear under hamburger dropdown.

cabab54

Fix #348

binh-dam-ibigroup merged commit c121587 into dev Apr 13, 2021

binh-dam-ibigroup deleted the mobile-batch-results branch April 13, 2021 17:44

github-actions bot added the released label Apr 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mobile batch results #339

Mobile batch results #339

binh-dam-ibigroup commented Mar 5, 2021 •

edited

Loading

binh-dam-ibigroup commented Mar 5, 2021

evansiroky Mar 11, 2021

binh-dam-ibigroup Mar 12, 2021

evansiroky left a comment

landonreed left a comment

landonreed Mar 22, 2021

binh-dam-ibigroup Mar 25, 2021

landonreed Mar 22, 2021

binh-dam-ibigroup Mar 24, 2021

binh-dam-ibigroup Mar 24, 2021

binh-dam-ibigroup Mar 24, 2021 •

edited

Loading

codecov-io commented Mar 23, 2021 •

edited

Loading

landonreed commented Apr 9, 2021

binh-dam-ibigroup commented Apr 9, 2021

landonreed Apr 9, 2021

binh-dam-ibigroup Apr 12, 2021

landonreed left a comment

evansiroky Apr 12, 2021

binh-dam-ibigroup Apr 13, 2021

evansiroky Apr 12, 2021

binh-dam-ibigroup Apr 13, 2021

evansiroky left a comment

github-actions bot commented Apr 13, 2021

Mobile batch results #339

Mobile batch results #339

Conversation

binh-dam-ibigroup commented Mar 5, 2021 • edited Loading

binh-dam-ibigroup commented Mar 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

landonreed 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

binh-dam-ibigroup Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Mar 23, 2021 • edited Loading

Codecov Report

landonreed commented Apr 9, 2021

binh-dam-ibigroup commented Apr 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed 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

evansiroky left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 13, 2021

binh-dam-ibigroup commented Mar 5, 2021 •

edited

Loading

binh-dam-ibigroup Mar 24, 2021 •

edited

Loading

codecov-io commented Mar 23, 2021 •

edited

Loading