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

Fix errors pop ups in map page #354

Merged
merged 11 commits into from
May 30, 2024
Merged

Fix errors pop ups in map page #354

merged 11 commits into from
May 30, 2024

Conversation

TheTexanCodeur
Copy link
Collaborator

@TheTexanCodeur TheTexanCodeur commented May 27, 2024

This PR fixes multiple issues linked to error pop ups in the map page. Here are the main fixs brought with this PR:

  1. Mutiple pop ups : Error pop ups are now filtered in order to only display relevant one once (no more multiple "no location" errors).

  2. Errors for user posts : Now, the map does't show an error when user checks the user posts tab with no location.

  3. Errors when location is re-enabled: no more error pop up occur when the location is re-enabled by the user.

The last point DOES NOT WORK on the emulator. Please use a real phone to test this bug fix.

First relevant commit: 0280f65

Acceptance criteria:

  • The map should not show an error pop-up when the user checks the user posts on the map with location disabled
  • A single error pop-up message appears when the location is disabled
  • Error messages do not appear once the location is enabled again

@TheTexanCodeur TheTexanCodeur changed the title Fix errors map Fix errors pop ups in map page May 27, 2024
@TheTexanCodeur TheTexanCodeur linked an issue May 27, 2024 that may be closed by this pull request
3 tasks
@TheTexanCodeur TheTexanCodeur marked this pull request as ready for review May 27, 2024 14:34
@TheTexanCodeur TheTexanCodeur self-assigned this May 27, 2024
@TheTexanCodeur TheTexanCodeur requested a review from Aderfish May 27, 2024 14:40
Copy link
Collaborator

@Aderfish Aderfish left a comment

Choose a reason for hiding this comment

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

Good job, I see that there are less problems while manually testing on my phone. I think you should add regression tests however.

The nearby posts posts page still throws an error after toggling the location, and it never reloads correctly. I have this problem on the emulator and my real phone.

location_toggle.webm

I think we should not show the current position marker in the my posts screen if we have location disabled, so as not to confuse the user. We could change the icon on the center button to show that we don't have location as well.

lib/viewmodels/map/map_pin_view_model.dart Show resolved Hide resolved
@TheTexanCodeur
Copy link
Collaborator Author

Thanks for comment/suggestions!

I pushed a fix that should resolve the last issue.

Concerning the UI part, I think that the custom user feedback on the map when location is turned off is out of scope for this PR. The goal of this PR is to have stable error system. The features that you propose are not bug fixes in my opinion.

Don't get me wrong, I think that those suggested features are great and should be backlogged! It's just that I don't see them fit in this particular PR.

@TheTexanCodeur TheTexanCodeur requested a review from Aderfish May 28, 2024 09:39
Copy link
Collaborator

@Aderfish Aderfish left a comment

Choose a reason for hiding this comment

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

Good job fixing that last problem and adding tests.

I now found a new bug where the center button no longer works after toggling location. It works again after refresh of the map page so it's not too bad. I'll add it to the backlog with my suggestions for the my posts tab. This PR indeed already accomplishes its criteria and improves the app.

lib/viewmodels/map/map_pin_view_model.dart Show resolved Hide resolved
@TheTexanCodeur TheTexanCodeur merged commit 4c84b2f into main May 30, 2024
3 checks passed
@TheTexanCodeur TheTexanCodeur deleted the fix-errors-map branch May 30, 2024 07:27
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.

Bug: Wrong error popups in Map
2 participants