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

Merged
merged 10 commits into from
Oct 3, 2024
Merged

Remove use of isTransit() function #1267

merged 10 commits into from
Oct 3, 2024

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

amy-corson-ibigroup
amy-corson-ibigroup reviewed Oct 1, 2024
View reviewed changes
Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

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

Uh well it does work.... I will be honest I am missing the logic of why we need this in core utils? Why import a function when we can just use leg.isTransit

lib/components/narrative/default/itinerary-description.tsx
@@ -68,7 +65,7 @@ export function ItineraryDescription({ itinerary }: Props): JSX.Element {
let transitMode
itinerary.legs.forEach((leg) => {
const { duration, mode, rentedBike, rentedVehicle } = leg
if (isTransit(mode) && duration > primaryTransitDuration) {
if (isTransitLeg(leg) && duration > primaryTransitDuration) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this check a variable? It's used twice, here and 35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that too but decided against it because it's a simple check anyway

amy-corson-ibigroup
amy-corson-ibigroup approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@amy-corson-ibigroup amy-corson-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 guess

@miles-grant-ibigroup miles-grant-ibigroup removed their assignment Oct 2, 2024
@daniel-heppner-ibigroup daniel-heppner-ibigroup merged commit 4848925 into dev Oct 3, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amy-corson-ibigroup amy-corson-ibigroup amy-corson-ibigroup approved these changes

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

Assignees

@daniel-heppner-ibigroup daniel-heppner-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