-
Notifications
You must be signed in to change notification settings - Fork 5
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 Map Panning issue #32
Fix Map Panning issue #32
Conversation
- Added isFocused property. - Added isIdle state - Incorrectly used useMemo replaced with useEffect.
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@hayata-suenaga Is there a way I can test the changes in the package locally (using ts files as they are) |
To do this, you can
I know this is such a cumbersome process 😭 |
@gegham-khachatryan Can you please resolve conflicts? |
@gegham-khachatryan let's solve the merge conflict 😄 @allroundexperts I think this one is ready for review 👍 |
I'm solving the conflict now so that @allroundexperts can start testing |
@allroundexperts this one is ready for test now 🙇 |
New RequirementActually, 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 🙇 |
@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. |
Hi @hayata-suenaga . Of course, I'll do that. |
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. Thanks |
@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 |
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 |
@hayata-suenaga Changes that This is the change causing the issue. |
Should we close this PR in favor of Expensify/App#25977 @hayata-suenaga @gegham-khachatryan? |
Details
Related Issues
Expensify/App#25732
Manual Tests
Linked PRs
Expensify/App#25977