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

Upgrade to latest react-map-gl #479

Closed
wants to merge 18 commits into from
Closed

Upgrade to latest react-map-gl #479

wants to merge 18 commits into from

Conversation

0x41head
Copy link

@0x41head 0x41head commented May 29, 2024 •
edited
Loading

closes #476

  • update to map from ReactMapGL
  • removed LinearInterpolator
  • Fixed WebMercatorViewport import

@0x41head 0x41head marked this pull request as draft May 29, 2024 17:25
@0x41head 0x41head marked this pull request as ready for review May 30, 2024 15:19
@0x41head 0x41head changed the title [WIP] Upgrade to latest react-map-gl Upgrade to latest react-map-gl May 30, 2024
@adeebshihadeh
Copy link
Contributor

Getting a bunch of errors when trying to run:
image

@0x41head
Copy link
Author

0x41head commented May 30, 2024 •
edited
Loading

Made some changes that should theoretically fix this issue. If for some reason it still persists, please inform me about your browser and browser version, so that I can test it there

@adeebshihadeh
Copy link
Contributor

Still doesn't work. Wonder why the tests pass?

Google Chrome	120.0.6099.129 (Official Build) (64-bit) 
Revision	204016af461e03117aa3858b51ba4534c7cfda81-refs/branch-heads/6099_110@{#3}
OS	Linux
JavaScript	V8 12.0.267.10
User Agent	Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36

@0x41head
Copy link
Author

Cannot replicate the error, even on the same browser version :/
Made a few changes that could help.
Is there anything specific you are doing to cause this issue or is it directly on boot ?
Do you have any extension that could potentially cause this issue ? Maybe try once on incognito ?

@0x41head
Copy link
Author

Nevermind, I was able to replicate the issue somehow. Working on a fix rn

@0x41head
Copy link
Author

Ready for review.

@adeebshihadeh
Copy link
Contributor

Works a little more but immediately hit a visual bug. All these things should be caught before the PR is marked ready for review. Try to break it before marking as ready for review again.

image

@adeebshihadeh adeebshihadeh marked this pull request as draft May 31, 2024 20:30
@adeebshihadeh adeebshihadeh marked this pull request as draft May 31, 2024 20:30
@0x41head
Copy link
Author

0x41head commented Jun 1, 2024 •
edited
Loading

Apologies for the regular pings.
I think a lot of these oversights are happening because I am using the demo account, while you are using an actual comma device. In order to actually verify a lot of these issues, I have to remove the conditional rendering happening at multiple places. For example, the hasNav variable only gets defined if you actually have a prime device with navigation feature
On a side note, we should probably try to integrate these into the unit test as well for easier development in the future.

@0x41head 0x41head marked this pull request as ready for review June 2, 2024 10:49
@adeebshihadeh
Copy link
Contributor

Can you rebase to get the new preview deployment? Then you can share the link on our Discord and have some people stress test it.

adeebshihadeh
adeebshihadeh requested changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

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

Can you clean this up? It's got debug stuff leftover and a bunch of inconsistent style

src/index.css Outdated Show resolved Hide resolved
src/components/Navigation/index.jsx Outdated Show resolved Hide resolved
package.json
Comment on lines +32 to +33
"@math.gl/core": "^4.0.1",
"@math.gl/web-mercator": "^4.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need new dependencies?

Copy link
Author

Choose a reason for hiding this comment

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

from react-map-gl upgrade docs:

viewport-mercator-project (an alias of @math.gl/web-mercator) is no longer a dependency. You can still install the library on the side as a utility for viewport-related math, but it's no longer required.

Copy link
Author

Choose a reason for hiding this comment

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

Reference

src/components/DriveMap/index.jsx Outdated Show resolved Hide resolved
src/components/DriveMap/index.jsx Outdated Show resolved Hide resolved
src/components/Navigation/index.jsx Outdated Show resolved Hide resolved
src/components/Navigation/index.jsx Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 6, 2024 •
edited
Loading

Welcome to connect! Make sure to:

  • read the contributing guidelines
  • mark your PR as a draft until it's ready to review
  • post the preview on Discord; feedback from users will speedup the PR review

deployed preview: https://479.connect-d5y.pages.dev

@adeebshihadeh
Copy link
Contributor

I'm closing out this bounty, since we're shifting our focus over to new-connect.

The work is done though, so you're good to collect the bounty. Instructions here: https://github.com/commaai/openpilot/blob/master/docs/BOUNTIES.md

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

@adeebshihadeh adeebshihadeh adeebshihadeh requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update to latest react-map-gl
2 participants
@0x41head @adeebshihadeh