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

Stop scroll capture on crash map and upgrade react-map-gl to v7 #1398

Merged
merged 35 commits into from
Mar 22, 2024

Conversation

mddilley
Copy link
Contributor

@mddilley mddilley commented Mar 14, 2024

Associated issues

cityofaustin/atd-data-tech#15867
cityofaustin/atd-data-tech#15190

Please review these PRs in this order: #1398, #1399, #1400

This PR:

  • updates react-map-gl to v7 and mapbox-gl as well
  • removes react-map-gl-draw and the LocationEditMap component that used it. This feature/component was intended to allow VZ users to edit location polygon which was never needed.
  • updates the browserslist config in package.json to address a JavaScript transpile error related to Mapbox GL

Testing

URL to test:
https://deploy-preview-1398--atd-vze-staging.netlify.app/

Steps to test:

  1. Go to a crash page and make sure the map loads
  2. See that there are zoom and fullscreen controls just like in staging and production
  3. Try scrolling while hovering the mouse over the crash map. You should see a prompt to use a cooperative gesture to zoom the map in and out.
  4. When the crash page loads, make sure that the map and the tile layer adjust to different heights of crash diagram to the right since they are in the same flex row. If they don't adjust correctly, you will see that the map tiles look partially loaded with some white space in the bottom portion. We don't want that!
  5. Explore and drag the map around, and you can compare side-by-side with staging or production too. Zoom out and notice that you can't explore the entire planet - just within a bounding box that I stole from Moped.
  6. You should expect the crash edit and location maps to not load. They are commented out and are updated in the next two PRs, and all of this will merge in together.

Ship list

  • Check migrations for any conflicts with latest migrations in master branch
  • Confirm Hasura role permissions for necessary access
  • Code reviewed
  • Product manager approved

@mddilley mddilley added the WIP Work in progress label Mar 14, 2024
This was referenced Mar 14, 2024
Copy link
Contributor Author

@mddilley mddilley Mar 15, 2024

Choose a reason for hiding this comment

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

This looks like a lot of change because it used to be a class component. The big changes are that shared config between all the maps was extracted into map.js. The old code also manages viewport state which isn't needed in the newer react-map-gl API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here - class component made into a function component.

Comment on lines +17 to +19
transform: `translate(0px, ${
isDragging && animated ? `-${size}px` : `${-size / 2}px`
})`,
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 had to update this style because the map was rendering the pin marker way off center with the old transform/translate values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the location polygon editor component. Not needed. 🗑️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The toolbar only used by LocationEditMap. 🗑️

@mddilley mddilley removed the WIP Work in progress label Mar 15, 2024
@@ -49,8 +49,7 @@
"react-csv-reader": "^3.0.6",
"react-datepicker": "^2.9.6",
"react-dom": "^16.8.6",
"react-map-gl": "^5.1.1",
"react-map-gl-draw": "^0.15.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This package was only used by the unneeded LocationEditMap component removed in this PR. It isn't compatible with the new react-map-gl version anyways.

Copy link
Member

Choose a reason for hiding this comment

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

✌️

Copy link
Member

@frankhereford frankhereford left a comment

Choose a reason for hiding this comment

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

This looks great Mike. I've read through the code and tested it with your really helpful and clear PR comment / testing steps. It's all 🚢 🚀 . I'm looking forward to reading the next PR!

I especially enjoyed reading about the browser coverage, and that's a really cool site you linked to show how they compared. Lots of TIL in there for me too.

padding: "10px",
};

export default class CrashMap extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for taking the time to turn this into a functional component! 🙏 🙏

Copy link
Member

Choose a reason for hiding this comment

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

+1 🙌

touchPitch: false,
dragRotate: false,
boxZoom: false,
maxBounds: [[-99, 29], [-96, 32]],
Copy link
Contributor

Choose a reason for hiding this comment

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

love the touch of making a bounding box so you cant go exploring the whole planet anymore!

Copy link
Member

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@roseeichelmann roseeichelmann left a comment

Choose a reason for hiding this comment

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

Thanks for working on this Mike, everything looks great! +1 thanks for turning those classes into functional components and rly like the touch of the bounding box 🚢

Copy link
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

this is looking so good! i'm moving on to the related PRs 🍿 🙌 🚢

@@ -49,8 +49,7 @@
"react-csv-reader": "^3.0.6",
"react-datepicker": "^2.9.6",
"react-dom": "^16.8.6",
"react-map-gl": "^5.1.1",
"react-map-gl-draw": "^0.15.0",
Copy link
Member

Choose a reason for hiding this comment

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

✌️

touchPitch: false,
dragRotate: false,
boxZoom: false,
maxBounds: [[-99, 29], [-96, 32]],
Copy link
Member

Choose a reason for hiding this comment

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

💯

// refetchCrashData={refetch}
// setIsEditingCoords={setIsEditingCoords}
// />
null}
Copy link
Member

Choose a reason for hiding this comment

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

noting that this is restored in a related branch of this work

padding: "10px",
};

export default class CrashMap extends Component {
Copy link
Member

Choose a reason for hiding this comment

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

+1 🙌

@mddilley mddilley merged commit 81c99a1 into master Mar 22, 2024
8 checks passed
@mddilley mddilley deleted the md-15867-stop-scroll-capture branch March 22, 2024 18:16
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.

4 participants