Skip to content

Conversation

@binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Aug 7, 2020

This PR adds a component that displays the real-time status of a transit leg.

To test

To test that the slot is being applied:

To test real-time updates:

  • in trimet-mod-otp branch master, insert in the package.json dependencies:
"@opentripplanner/itinerary-body": "^1.1.0",
  • in trimet-mod-otp/node_modules/otp-react-redux/node_modules, delete the itinerary-body folder.
  • From the configurations repo, execute otp/run.js commtrans. Find an itinerary with real-time updates, and observe the updated leg times per the screenshot below.

image

@binh-dam-ibigroup binh-dam-ibigroup added the WIP Work in progress label Aug 7, 2020
@binh-dam-ibigroup binh-dam-ibigroup marked this pull request as draft August 7, 2020 22:53
@binh-dam-ibigroup binh-dam-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Aug 14, 2020
@binh-dam-ibigroup binh-dam-ibigroup changed the title WIP: Display transit leg real-time status Display transit leg real-time status Aug 18, 2020
@binh-dam-ibigroup binh-dam-ibigroup added WIP Work in progress BLOCKED Blocked (waiting on another PR to be merged) and removed BLOCKED Blocked (waiting on another PR to be merged) WIP Work in progress labels Aug 18, 2020
@binh-dam-ibigroup binh-dam-ibigroup marked this pull request as ready for review August 19, 2020 16:42
<TimeColumn>
{renderedTime}
<StatusText>
{!isOnTime && <>{delayInMinutes}&nbsp;min</>}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be cleaner to use a template string here instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 2115c7b.


// TODO: refine on-time thresholds.
// const isOnTime = delay >= -60 && delay <= 120;
const isOnTime = delay === 0
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine this on time calculation might vary from deployment-to-deployment, so I'm not going to bother speculating how we should change this until we need to.

Copy link
Contributor

@evansiroky evansiroky left a comment

Choose a reason for hiding this comment

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

Looks good... once the merge conflicts are resolved.

@binh-dam-ibigroup binh-dam-ibigroup removed their assignment Aug 25, 2020
@binh-dam-ibigroup
Copy link
Collaborator Author

The merge conflicts the templates string suggestion have been addressed. I also updated the PR description for testing.

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

A couple of changes needed, but otherwise this is looking good. Also, could you update the testing instructions for running the otp-rr example? In general, I don't think it's productive to try to do the testing of this library through trimet-mod-otp (unless it's really needed).

* This component displays the scheduled departure/arrival time for a leg,
* and, for transit legs, displays any delays or earliness where applicable.
*/
export default function TimeColumnWithDelays ({
Copy link
Member

Choose a reason for hiding this comment

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

The name of this should be RealTimeColumn or something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 3ef29f3

{/* Add the scheduled mention for transit legs only. */}
{isTransitLeg && <StatusText>scheduled</StatusText>}
</>
)
Copy link
Member

Choose a reason for hiding this comment

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

This should go above everything else in a conditional. Otherwise, this ends up getting lost in the shuffle. It's like placing the most important sentence at the very end of five pages of text. (This is a bad analogy, but you catch my drift.)

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 comment or the entire return statement?

Copy link
Member

Choose a reason for hiding this comment

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

The entire statement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed in 3ef29f3

<StatusText>
{/* Keep the '5 min' string on the same line. */}
{/* Append space before printing statusText. (statusText can be displayed on same line or new line.) */}
{!isOnTime && `${delayInMinutes}\xa0min `}
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't see this earlier. I think it would be better to wrap the delay minutes text in a div styled with white-space: nowrap;. The \xa0 character is cryptic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 60f4ad6

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Looks good, just need to replace the \xa0 char.

{renderedTime}
<StatusText>
{/* Keep the '5 min' string on the same line. */}
{!isOnTime && <DelayText>{`${delayInMinutes} min`}</DelayText>}
Copy link
Member

Choose a reason for hiding this comment

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

We can remove the template string now I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 83c2012

Copy link
Member

@landonreed landonreed left a comment

Choose a reason for hiding this comment

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

Sorry, minor change with the new DelayText component.

@landonreed landonreed merged commit 47af44f into dev Sep 1, 2020
@landonreed landonreed deleted the realtime-time-update branch September 1, 2020 21:53
@binh-dam-ibigroup binh-dam-ibigroup mentioned this pull request Sep 8, 2020
@landonreed
Copy link
Member

🎉 This PR is included in version 1.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@landonreed
Copy link
Member

🎉 This PR is included in version 1.0.0-alpha.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants