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

Fix Map Panning issue #32

Closed
wants to merge 4 commits into from
Closed

Fix Map Panning issue #32

wants to merge 4 commits into from

Conversation

gegham-khachatryan
Copy link

Details

  • Added isFocused property.
  • Added isIdle state
  • Incorrectly used useMemo replaced with useEffect.
  • Added new props to avoid introducing unnecessary dependencies ('@react-navigation/native') to the 'react-native-x-maps' library.

Related Issues

Expensify/App#25732

Manual Tests

  1. Click the "+" icon next to the chat message text field. Click "Request Money" on the pop up menu that appears.
  2. Check "Distance" tab on the Right Hand Panel that appears
  3. Click the "Start" item from the list of waypoints.
  4. In the text field that appears in the next screen, type "88 Kearny Street". Click the first option "88 Kearny Street, San Francisco, CA, USA" that appears.
  5. Map camera flied to selected address where marker is visible.
  6. Click Add Step button
  7. Click on new added step
  8. In the text field that appears in the next screen, type "Golden Gate Park". Click the first option "Golden Gate Pkwy, Naples, FL, USA" that appears.
  9. Verify that the 2 markers are on the map and camera moved to show both on the screen.
  10. Added Finish step
  11. Verify that the distance was drawn as expected.

Linked PRs

Expensify/App#25977

@gegham-khachatryan
Fix Map Panning issue
56fbb25 
- Added isFocused property.
- Added isIdle state
- Incorrectly used useMemo replaced with useEffect.
@gegham-khachatryan
Rename callback method by policy
450633b
@github-actions
Copy link
Contributor

github-actions bot commented Aug 25, 2023 •
edited
Loading

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@gegham-khachatryan
Copy link
Author

I have read the CLA Document and I hereby sign the CLA

@allroundexperts
Copy link

allroundexperts commented Aug 27, 2023 •
edited
Loading

@hayata-suenaga Is there a way I can test the changes in the package locally (using ts files as they are)

@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Aug 28, 2023 •
edited
Loading

@hayata-suenaga Is there a way I can test the changes in the package locally (using ts files as they are)

@allroundexperts

To do this, you can

  1. Pull @gegham-khachatryan's PR
  2. Create a new branch off the PR branch
  3. Remove dist from gitignore
  4. Run npm run build
  5. Commit the change and push your branch
  6. Use the format below to download the build in App from your branch on GitHub
    npm install git+https://github.com/user/repo.git#branch

I know this is such a cumbersome process 😭

@allroundexperts
Copy link

@gegham-khachatryan Can you please resolve conflicts?

@hayata-suenaga
Copy link
Contributor

@gegham-khachatryan let's solve the merge conflict 😄

@allroundexperts I think this one is ready for review 👍

@hayata-suenaga hayata-suenaga mentioned this pull request Aug 29, 2023
Closed
59 tasks
@hayata-suenaga
Copy link
Contributor

I'm solving the conflict now so that @allroundexperts can start testing

@hayata-suenaga
Copy link
Contributor

@allroundexperts this one is ready for test now 🙇

@hayata-suenaga hayata-suenaga requested review from neil-marcellini and removed request for neil-marcellini August 30, 2023 01:10
@hayata-suenaga
Copy link
Contributor

New Requirement

Actually, we have a new requirement:

Let's just move the whole code to App. We decided to sunset this library and move this code over to App because we just ended up exposing a small portion of Mapbox's feature from this library.

App is already setup to handle TypeScript, so we can mostly just copy and paste the code.

@gegham-khachatryan I haven't heard back from you for my question. If we don't hear back from you by August 30 9am Pacific Day Time, we gonna assign a different contributor or handle this issue internally 🙇

@akinwale
Copy link

@hayata-suenaga If this ends up requiring reassignment, I'm available to handle it since I found the root cause and I'm very familiar with the code. Thanks.

@gegham-khachatryan
Copy link
Author

Actually, we have a new requirement:

Let's just move the whole code to App. We decided to sunset this library and move this code over to App because we just ended up exposing a small portion of Mapbox's feature from this library.

App is already setup to handle TypeScript, so we can mostly just copy and paste the code.

Hi @hayata-suenaga . Of course, I'll do that.

@hayata-suenaga
Copy link
Contributor

oh nice thank you @gegham-khachatryan for quick response and work 🚀 💯 🙇

@gegham-khachatryan
Copy link
Author

gegham-khachatryan commented Aug 30, 2023 •
edited
Loading

oh nice thank you @gegham-khachatryan for quick response and work 🚀 💯 🙇

@hayata-suenaga All code moved to the app, fixed linter issues, tested on native android and iOS but had problems with testing on the web. The problem was mapboxAccessToken, maybe I did not configure correctly for web, but after passing it directly (only for local testing) Map started work properly. PR updated.
Expensify/App#25977

Thanks

@gegham-khachatryan
Copy link
Author

@hayata-suenaga @allroundexperts I've uncovered a critical issue causing the app to crash when removing waypoints. This problem arises after merging changes from the main branch into my own branch. Importantly, the main branch is also affected.

The root of the issue appears to be in the "Transaction.js" file at line 97, consistently triggering the crash.

Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2023-08-30.at.13.10.45.mp4

@hayata-suenaga
Copy link
Contributor

hayata-suenaga commented Aug 30, 2023 •
edited
Loading

thank you very much for finding the regression. Is it possible for you to handle the issue you discovered in this PR?

@gegham-khachatryan also let's move the conversation to this PR as we're going to close this PR anyway

@gegham-khachatryan
Copy link
Author

thank you very much for finding the regression. Is it possible for you to handle the issue you discovered in this PR?

@gegham-khachatryan also let's move the conversation to this PR as we're going to close this PR anyway

Let's discuss about this issue, under the PR

@gegham-khachatryan
Copy link
Author

gegham-khachatryan commented Aug 30, 2023 •
edited
Loading

thank you very much for finding the regression. Is it possible for you to handle the issue you discovered in this PR?

@gegham-khachatryan also let's move the conversation to this PR as we're going to close this PR anyway

@hayata-suenaga Changes that This is the change causing the issue.

@hayata-suenaga hayata-suenaga mentioned this pull request Aug 30, 2023
Closed
6 tasks
@neil-marcellini
Copy link
Contributor

Should we close this PR in favor of Expensify/App#25977 @hayata-suenaga @gegham-khachatryan?

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

@hayata-suenaga hayata-suenaga hayata-suenaga left review comments

@allroundexperts allroundexperts Awaiting requested review from allroundexperts

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

Successfully merging this pull request may close these issues.
5 participants
@gegham-khachatryan @allroundexperts @hayata-suenaga @akinwale @neil-marcellini