Improve trip settings #323

binh-dam-ibigroup · 2021-02-02T23:19:54Z

This PR makes the following changes to the trip settings:

The screen scrolls to trip name/days if the user is submitting the form and there is a validation error on one of these fields.
The trip notification settings controls are reorganized per mockup document.
A single field in the UI sets both the departureVarianceMinutesThreshold and arrivalVarianceMinutesThreshold.

…ose are invalid.

…itor.

landonreed · 2021-02-04T15:24:47Z

lib/components/user/monitored-trip/trip-notifications-pane.js

-                onChange={this._handleIsActiveChange}
-                value
+          <h4>Notify me via {notificationChannelLabels[notificationChannel]} when:</h4>
+          <SettingsListWithAlign>


After seeing this in action, let's make all these settings flush with the left edge (i.e., no indentation). Same goes for the below item in Advanced Settings.

Updated in 4801a00.

landonreed · 2021-02-04T15:31:41Z

lib/components/user/monitored-trip/trip-basics-pane.js

-      }
-    }
+    this._updateNewTripItineraryExistence(prevProps)
+    this._scrollToTripDataIfInvalid()


I somewhat expected formik to have something to handle this. Perhaps this approach could be leveraged (it uses hooks, so maybe not)? jaredpalmer/formik#146 (comment). This is not blocking by any means. What you have already makes sense... just exploring.

Also, similar to how we highlight the stop viewer row on scroll, it might be good to highlight/fade the invalid items too (probably in the alert-danger bg color).

The scrolling is not built-in, so there are code snippets and even a library. Library is cleaner. 4801a00
Errors were already highlighted in standard Bootstrap red.

landonreed · 2021-02-04T15:59:09Z

lib/components/user/monitored-trip/trip-basics-pane.js

          <FormGroup validationState={tripNameValidationState}>
-            <ControlLabel>Please provide a name for this trip:</ControlLabel>
+            <ControlLabel>
+              {/* Put a ref in a native DOM element for access to scroll function. */}


This comment feels a little awkward. Maybe:

Put a ref in this native DOM element to scroll to if trip name is invalid.

landonreed · 2021-02-04T16:03:08Z

lib/components/user/stacked-pane-display.js

-    {title && <PageHeading>{title}</PageHeading>}
+    {title && (
+      <PageHeading>
+        <Button bsStyle='primary' className='pull-right' type='submit'>Save</Button>


I'm not a huge fan of the Save button up here (at least for now). I think we want the user to take the action of scrolling through the form to view/confirm the settings before saving.

That was an experiment... It's reverted by 8dc5971.

landonreed · 2021-02-04T16:09:12Z

lib/components/user/monitored-trip/trip-notifications-pane.js

+                  name='leadTimeInMinutes'
+                >
+                  <option value={15}>15 min</option>
+                  <option value={30}>30 min (default)</option>


Because it's not the first option, 30 minutes is not selected by default. For this select and the others, we may want to add a selected prop? I'm not too familiar with how Formik handles <select/> inputs though, so I'm open to alternative suggestions.

Forgot about that, glad you pointed it out.

That was trickier. The default value was already populated correctly in the trip object, but the I passed the wrong component for styling, so it's fixed now in 4801a00.

landonreed

Looks good! Just a couple of styling tweaks and an issue with the dropdown selects (selected prop might be needed).

…top." This reverts commit 42a08c1.

evansiroky · 2021-02-19T23:44:16Z

lib/components/user/monitored-trip/trip-notifications-pane.js

+            <li>
+              <Select
+                label='There are delays or disruptions of more than'
+                name='departureVarianceMinutesThreshold'


I'm just noticing there is no field for arrivalVarianceMinutesThreshold. Should there be?

@evansiroky In b30a2ef I split the target threshold field depending on whether the trip is arriveBy.

Hmm... I'm somewhat of the mind that we should just set both thresholds to the same value (chosen by the single select component here) regardless of whether the trip is arriveBy/departAt. If I had a trip that was set to arrive by 9am. I would still really like to know if the first leg is running early by 15 minutes (an extreme case, but still feasible). Thoughts?

I am in favor of using a single selector, so @landonreed's idea may be better overall?

Per team discussion, I implemented single UI field that sets the value for both the departure and arrival delay thresholds in 2dd1821.

evansiroky · 2021-02-26T18:30:29Z

Assigning back to Binh to fix merge conflicts and also update per our weekly technical meeting.

…e and arrival delay thres.

landonreed

Just a couple of minor comments.

landonreed · 2021-03-03T20:41:56Z

lib/components/user/account-page.js

 import { Col, Row } from 'react-bootstrap'
 import { connect } from 'react-redux'

 import * as uiActions from '../../actions/ui'


Why are trips needed here?

It's leftover orphan code, removed in 814d9df.

landonreed · 2021-03-03T20:49:08Z

lib/components/user/monitored-trip/trip-notifications-pane.js

                  <option value={60}>1 hour</option>
                </Select>
-                before it begins until it ends.
+                before it begins, until it ends.


Not sure about the inclusion of this comma. I would say we should probably remove it.

Removed in 814d9df.

landonreed · 2021-03-03T20:51:26Z

lib/components/user/monitored-trip/saved-trip-screen.js

+      clonedMonitoredTrip.delayThreshold = Math.min(
+        monitoredTrip.arrivalVarianceMinutesThreshold,
+        monitoredTrip.departureVarianceMinutesThreshold
+      )


Maybe all of this logic (cloning and setting delay field) should happen in a method prepareTripForEditing? Alternatively, if there's a way to just set both of these values using Formik (rather than manipulating the object), that might be preferable.

Updated in 814d9df.

…trip notif. pane.

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

🎉 This PR is included in version 3.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

binh-dam-ibigroup added 4 commits February 2, 2021 10:33

improvement(TripBasicsPane): Scroll to trip name/days on submit if th…

5a25d9c

…ose are invalid.

refactor(TripNotificationsPane): Update styles.

c6cf7dc

refactor(TripNotificationsPane): Improve semantics.

41084cd

fix(TripNotificationsPane): Add toggle for advanced trip settings.

0acd10d

binh-dam-ibigroup requested review from evansiroky and landonreed February 2, 2021 23:19

binh-dam-ibigroup assigned landonreed and evansiroky Feb 2, 2021

binh-dam-ibigroup added 6 commits February 2, 2021 18:30

fix(TripNotificationsPane): Add aria props to advanced settings toggle.

a990abc

refactor(AccountPage): Remove unused prop.

337bb96

refactor(BackLink): Extract back button link.

f7efc70

refactor(BackLink): Move markup into BackLink

6c40335

refactor(BackLink): Replace back to planner with back link in trip ed…

77affe0

…itor.

refactor(StackedPaneDisplay): Experiment with Save button on top.

42a08c1

landonreed reviewed Feb 4, 2021

View reviewed changes

landonreed requested changes Feb 4, 2021

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Feb 4, 2021

binh-dam-ibigroup added 2 commits February 5, 2021 11:14

Revert "refactor(StackedPaneDisplay): Experiment with Save button on …

8dc5971

…top." This reverts commit 42a08c1.

refactor(Trip panes): Address PR comments

4801a00

binh-dam-ibigroup requested a review from landonreed February 5, 2021 17:32

binh-dam-ibigroup assigned landonreed and unassigned binh-dam-ibigroup Feb 5, 2021

landonreed approved these changes Feb 12, 2021

View reviewed changes

landonreed removed their assignment Feb 12, 2021

evansiroky reviewed Feb 19, 2021

View reviewed changes

binh-dam-ibigroup assigned evansiroky and unassigned binh-dam-ibigroup Feb 22, 2021

evansiroky assigned binh-dam-ibigroup and unassigned evansiroky Feb 26, 2021

binh-dam-ibigroup added 2 commits March 1, 2021 16:44

refactor(TripNotificationPane): Use delayThreshold to manage departur…

2dd1821

…e and arrival delay thres.

Merge branch 'dev' into improve-trip-name-validation

79f1545

binh-dam-ibigroup requested a review from landonreed March 1, 2021 21:56

binh-dam-ibigroup assigned landonreed and evansiroky and unassigned binh-dam-ibigroup Mar 1, 2021

landonreed requested changes Mar 3, 2021

View reviewed changes

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

refactor: Address PR comments

814d9df

binh-dam-ibigroup requested a review from landonreed March 4, 2021 16:46

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

landonreed approved these changes Mar 4, 2021

View reviewed changes

landonreed removed their assignment Mar 4, 2021

binh-dam-ibigroup added 2 commits March 4, 2021 12:27

refactor(TripNotificationsPane): Move common delayThreshold logic to …

2487512

…trip notif. pane.

refactor(TripNotificationPane): Add comments, rename vars.

ec90a40

evansiroky approved these changes Mar 11, 2021

View reviewed changes

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

binh-dam-ibigroup merged commit d10926c into dev Mar 12, 2021

binh-dam-ibigroup deleted the improve-trip-name-validation branch March 12, 2021 15:08

evansiroky mentioned this pull request Mar 31, 2021

Next release #351

Merged

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

Improve trip settings #323

Improve trip settings #323

Uh oh!

Conversation

binh-dam-ibigroup commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

landonreed Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

landonreed Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

binh-dam-ibigroup Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

landonreed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evansiroky commented Feb 26, 2021

Uh oh!

landonreed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

binh-dam-ibigroup commented Feb 2, 2021 •

edited

Loading

landonreed Feb 4, 2021 •

edited

Loading

landonreed Feb 4, 2021 •

edited

Loading

binh-dam-ibigroup Feb 5, 2021 •

edited

Loading