Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support one-time trips #1186

Merged
merged 18 commits into from
May 3, 2024
Merged

Support one-time trips #1186

merged 18 commits into from
May 3, 2024

Conversation

binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Apr 1, 2024

Description

Works with ibi-group/otp-middleware#224

This PR adds support for saving/monitoring trips that will be taken only once.

Behaviors of note:

  • Checking the "Only on <date>" checkbox will uncheck other monitored days. Checking a monitored day will uncheck "Only on <date>".
  • For one-time trips in the "trip is active" state, the state will switch to "Trip in the past" after the trip end time (with real-time adjustments if applicable).
  • The requirement to have at least one monitored day between Monday and Sunday is removed.

PR Checklist

  • Does the code follow accessibility standards (WCAG 2.1 AA Compliant)?
  • Are all languages supported (Internationalization/Localization)?
  • Are appropriate Typescript types implemented?
Before After
Before screenshot after screenshot

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the new code looks good, just one small nit about the UI for the new feature

Comment on lines 272 to 277
<Checkbox checked={isOneTime} onChange={this._handleOneTimeTrip}>
<FormattedMessage
id="components.TripBasicsPane.onlyOnDate"
values={{ date: itinerary.startTime }}
/>
</Checkbox>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this maybe be a radio button instead? I'm thinking of having a big toggle switch between "monitor once" and "monitor regularly" and that way we can hide the calendar view if we have to

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using radio buttons in 29567d4. I added the hide thing in d391207. I think that makes things clearer.

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good enough for now!

export default function pastTripRenderer(
monitoredTrip: MonitoredTrip
): MonitoredTripRenderData {
const data = baseRenderer(monitoredTrip) as unknown as MonitoredTripRenderData
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, what's going on with the type here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I just write as MonitoredTripRenderData, TypeScript doesn't believe me...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, the file it's in isn't typescripted.

<FormattedMessage id="components.TripBasicsPane.recurringEachWeek" />
</Radio>
{!isOneTime ? (
<>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to move each side of this ternary into a component so that it's easier to read? I had trouble finding the else part of the ternary, it's a lot of open and close brackets. It's fine if the components are inside this one so there are no props.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map in there is also a lot to parse. Feels like a bit too much to tuck inline.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I just understood that this ternary is being used to just hide this when isOneTime is true. Could it just be !isOneTime && in that case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map in there is also a lot to parse. Feels like a bit too much to tuck inline.

There is a redesign of this portion upcoming, so we will address that at that time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it just be !isOneTime && in that case?

Refactored in 655454c.

Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, pending some kind of correction to the current behavior which breaks if the middleware is unable to validate a trip exists.

@binh-dam-ibigroup
Copy link
Collaborator Author

This looks good, pending some kind of correction to the current behavior which breaks if the middleware is unable to validate a trip exists.

@daniel-heppner-ibigroup Let me know what you think of this fix 655454c.

Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you! @binh-dam-ibigroup

@binh-dam-ibigroup binh-dam-ibigroup merged commit 5f21292 into dev May 3, 2024
9 checks passed
@binh-dam-ibigroup binh-dam-ibigroup deleted the one-time-trips branch May 3, 2024 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants