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.

Sign up for GitHub

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

Jump to bottom

Remove use of isTransit() function #1267

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Remove use of isTransit() function #1267

wants to merge 8 commits into from

Conversation

daniel-heppner-ibigroup
Copy link
Contributor

Description:
The isTransit() function is unneeded because we can just use the transitLeg propery on legs. It was causing bugs with some of our custom modes.

@daniel-heppner-ibigroup
Copy link
Contributor Author

I also deleted a file that wasn't imported anywhere. Did we mean to be using that?

@miles-grant-ibigroup
Copy link
Collaborator

I also deleted a file that wasn't imported anywhere. Did we mean to be using that?

That's from the tabbed UI. Nobody that I know of currently uses that but technically it's still supported? What do we think

miles-grant-ibigroup
miles-grant-ibigroup approved these changes Sep 10, 2024
View reviewed changes
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.

We're really on OTP2 now... eek!

miles-grant-ibigroup
miles-grant-ibigroup requested changes Sep 10, 2024
View reviewed changes
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.

Updating my review here to continue using the isTransit method in core-utils instead. Let's refactor that! This PR shouldn't be closed though I like the cleanup

@daniel-heppner-ibigroup
Copy link
Contributor Author

I also deleted a file that wasn't imported anywhere. Did we mean to be using that?

That's from the tabbed UI. Nobody that I know of currently uses that but technically it's still supported? What do we think

Well it wasn't even exported in index.js

@miles-grant-ibigroup
Copy link
Collaborator

I also deleted a file that wasn't imported anywhere. Did we mean to be using that?

That's from the tabbed UI. Nobody that I know of currently uses that but technically it's still supported? What do we think

Well it wasn't even exported in index.js

Let's just update core-utils instead

@daniel-heppner-ibigroup daniel-heppner-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Sep 18, 2024
@daniel-heppner-ibigroup
Copy link
Contributor Author

daniel-heppner-ibigroup commented Sep 18, 2024 •
edited
Loading

Will block this one until the OTP UI side is merged

EDIT: ready to go!

@daniel-heppner-ibigroup daniel-heppner-ibigroup removed the BLOCKED Blocked (waiting on another PR to be merged) label Sep 23, 2024
miles-grant-ibigroup
miles-grant-ibigroup approved these changes Sep 25, 2024
View reviewed changes
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.

Swaggy

miles-grant-ibigroup
miles-grant-ibigroup requested changes Sep 25, 2024
View reviewed changes
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.

I trusted you! Post lunch I'm seeing this with new clairty

lib/actions/field-trip.js Outdated Show resolved Hide resolved
lib/components/narrative/default/default-itinerary.js Outdated Show resolved Hide resolved
lib/components/narrative/default/itinerary-description.tsx
(leg.transitLeg || isTransit(mode)) &&
duration > primaryTransitDuration
) {
if (isTransitLeg(leg) && duration > primaryTransitDuration) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm scared to remove the current check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

time to get over your fears

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤬

lib/components/narrative/default/itinerary-summary.js
itinerary.legs[i - 1].transitLeg &&
itinerary.legs[i + 1].transitLeg
!isTransitLeg(leg) &&
isTransitLeg(itinerary.legs[i - 1]) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to keep the old check maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe, but maybe not

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤬

lib/components/narrative/line-itin/LegIconWithA11y.tsx Outdated Show resolved Hide resolved
lib/components/narrative/line-itin/realtime-time-column.tsx Outdated Show resolved Hide resolved
lib/components/narrative/narrative-itineraries.js Outdated Show resolved Hide resolved
lib/util/itinerary.tsx Outdated Show resolved Hide resolved
@daniel-heppner-ibigroup
Copy link
Contributor Author

all the problems are fixed

miles-grant-ibigroup
miles-grant-ibigroup requested changes Sep 26, 2024
View reviewed changes
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.

It can be swaggier

lib/actions/field-trip.js Outdated Show resolved Hide resolved
lib/actions/field-trip.js Outdated Show resolved Hide resolved
lib/components/narrative/narrative-itineraries.js Outdated Show resolved Hide resolved
miles-grant-ibigroup
miles-grant-ibigroup approved these changes Sep 27, 2024
View reviewed changes
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.

Excited for the second reviewer

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

@miles-grant-ibigroup miles-grant-ibigroup miles-grant-ibigroup approved these changes

Assignees

@amy-corson-ibigroup amy-corson-ibigroup

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daniel-heppner-ibigroup @miles-grant-ibigroup @amy-corson-ibigroup