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

fix: add areaStops layer support #722

Merged

Conversation

miles-grant-ibigroup
Copy link
Collaborator

This add support for the new OTP areaStops layer. I feel like implementation is not perfect. Looking for advice

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Could you add a mock response and handler so we can see how the area could look like.

packages/otp2-tile-overlay/src/index.tsx Show resolved Hide resolved
@miles-grant-ibigroup
Copy link
Collaborator Author

Could you add a mock response and handler so we can see how the area could look like.

Do you have an idea of how to do this elegantly? The current mock response is a crazy hack that I'm not eager to recreate/expand

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed review, it took me a while decide to build an OTP instance with areaStops. It is nice that areas are showing without using too much code. I would reduce the opacity in the layers as indicated, so that map background features remain visible. How/where are circles supposed to be shown?

packages/otp2-tile-overlay/src/index.tsx Outdated Show resolved Hide resolved
packages/otp2-tile-overlay/src/index.tsx Outdated Show resolved Hide resolved
packages/otp2-tile-overlay/src/index.tsx Show resolved Hide resolved
packages/otp2-tile-overlay/src/index.tsx Outdated Show resolved Hide resolved
packages/otp2-tile-overlay/src/util.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Please see refactoring suggestion.

packages/otp2-tile-overlay/src/index.tsx Outdated Show resolved Hide resolved
refactor(otp2-tile-overlay): Refactor route color expression.
@miles-grant-ibigroup
Copy link
Collaborator Author

It's lovely thanks

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-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 better!

@miles-grant-ibigroup miles-grant-ibigroup removed their assignment Apr 19, 2024
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.

Just a question but otherwise looks all good!

packages/otp2-tile-overlay/src/index.tsx Show resolved Hide resolved
@miles-grant-ibigroup miles-grant-ibigroup merged commit 2b63aba into opentripplanner:master May 6, 2024
2 checks passed
@miles-grant-ibigroup miles-grant-ibigroup deleted the areaStops-support branch May 6, 2024 15:08
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