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

🏹 Implement Shuttle Route KML Visualization #996

Merged
merged 14 commits into from
Jul 31, 2024
Merged

Conversation

bfauble
Copy link
Contributor

@bfauble bfauble commented Jul 25, 2024

Summary of changes

Asana Ticket: 🏹 Implement Shuttle Route KML Visualization

image
image
image
image
image

Copy link

Coverage of commit 528f65c

Summary coverage rate:
  lines......: 81.7% (882 of 1079 lines)
  functions..: 62.4% (619 of 992 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow_web/controllers/shape_html.ex                            |66.7%      6|44.4%     9|    -      0

Download coverage report

@bfauble bfauble requested review from a team and jzimbel-mbta and removed request for a team July 26, 2024 14:06
Copy link

Coverage of commit 53f6fdf

Summary coverage rate:
  lines......: 81.7% (882 of 1079 lines)
  functions..: 62.4% (619 of 992 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow_web/controllers/shape_html.ex                            |66.7%      6|44.4%     9|    -      0

Download coverage report

Copy link
Member

@jzimbel-mbta jzimbel-mbta left a comment

Choose a reason for hiding this comment

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

A handful of minor things, otherwise looks good.

assets/src/components/ShapeViewMap.tsx Outdated Show resolved Hide resolved
assets/src/components/ShapeViewMap.tsx Outdated Show resolved Hide resolved
assets/src/components/ShapeViewMap.tsx Outdated Show resolved Hide resolved
lib/arrow_web/controllers/shape_html.ex Outdated Show resolved Hide resolved
coordinate_pair
|> String.split(",")
|> Enum.map(&String.to_float/1)
|> Enum.reverse()
Copy link
Member

Choose a reason for hiding this comment

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

Why does the pair need to be reversed? Is the coordinate_pair string formatted like "long,lat" for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, unsure why it's in this format

Copy link
Member

Choose a reason for hiding this comment

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

Weird!

Copy link
Member

Choose a reason for hiding this comment

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

Do you know where the coordinate pair string originates from? Wondering if this is something that's within our ability to fix.

If not, maybe just leave a comment above this line explaining why it needs to be reversed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my guess would be remix, the shape files come in with the pairs in long,lat format for some reason. the same is true for all the files currently in gtfs_creator

Copy link

Coverage of commit de3e01e

Summary coverage rate:
  lines......: 81.8% (883 of 1080 lines)
  functions..: 62.4% (620 of 993 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow_web/controllers/shape_html.ex                            |66.7%      6|44.4%     9|    -      0

Download coverage report

Copy link
Member

@jzimbel-mbta jzimbel-mbta left a comment

Choose a reason for hiding this comment

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

Good to go!

(Unless you think there's a way to fix the cause of the "Long,Lat" strings. Then it's good to go after you make that fix.)

Comment on lines +40 to +44
const PolyLines = ({ shapes }: { shapes: Shape[] }) =>
shapes.map((shape: Shape, index: number) => {
const key = crypto.randomUUID()
return <PolyLine shape={shape} index={index} key={key} keyPrefix={key} />
})
Copy link
Member

Choose a reason for hiding this comment

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

Just leaving a note here:
The randomly-generated keys should be fine in this case since we only expect this component to re-render (in other words, this function to run) when shapes changes, and when shapes changes it will be a completely different object since it describes a different route pattern.

So it's fine that all of this component's children will remount on re-render—we don't expect there to ever be partial changes to the shapes object.

@bfauble bfauble merged commit e6eb53c into master Jul 31, 2024
40 checks passed
@bfauble bfauble deleted the bf-shape-visualization branch July 31, 2024 16:51
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.

2 participants