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

refactor(map): change map & clustering package #1154

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

d4v3d4vE
Copy link
Contributor

@d4v3d4vE d4v3d4vE commented Jul 3, 2024

  • added maplibre-react-native through yarn add maplibre-react-native
    https://github.com/maplibre/maplibre-react-native
  • updated app.json according to maplibre-react-native
  • adjusted the code accordingly to keep all old map functionalities with the new map package '@maplibre/maplibre-react-native'
  • implemented a calculation mechanism to always show a map with multiple markers with the perfect positioning & zoom settings to display all markers on the map
  • ran yarn remove react-native-map-cluster to remove the package
  • ran yarn add supercluster to add the package
  • implemented new more efficient clustering via supercluster
  • added translucentAccent to colors.js for the cluster design
  • sorted imports
  • formatted code
  • removed prop isMultipleMarkersMap, since we calculate the exact camera positioning to show all markers on the map over the marker placements on the map to ensure always the perfect map view with all set markers
  • added old cluster design with a adjustable hitbox, set to 80
  • added the functionality to disable clustering, clustering is enabled by default
  • added back prop functionality to all used props to ensure the same map behaviour as before
  • added View to LocationSettings.js to ensure a reliable map behaviour
  • added new LoadingSinnerMap.tsx for a compatible loading spinner overlay as the map is loading
  • removed translucentAccent from colors.js as it's not needed anymore
  • adjusted map styling all over the projects used map components to ensure a consistent map behaviour
  • added normalize() throughout the files I worked in to ensure a consistent design
  • updated styles in Map.tsx to prevent the app from crashing on some screens with maps
  • added containerStyle prop to LoadingContainer
  • deleted because LoadingSpinnerMap was replaced with LoadingSpinner
  • added collapsable structure in LocationSettings again
  • added map style to SueMapScreen and SueMapViewScreen to get full screen map view
  • added onMarkerPress and selectedMarker to SueMapViewScreen and SueReportLocation to show callout text when marker is pressed
  • removed setTimouts because they caused unnecessary waiting while loading the map
  • unused props removed
  • added blank lines to the code to increase readability
  • corrected sorting errors
  • corrected some minor spelling errors
  • added setSelectedMarker(undefined) to onMapPress function in SueReportLocation and SueMapViewScreen to remove callout text from the screen
  • added calculateBoundsToFitAllMarkers helper to prevent the Map.tsx component from growing larger
  • updated the name of the calculateBounds helper to calculateBoundsToFitAllMarkers for clarity
  • added comment line to understand values in bounds array
  • increased the animationDuration value from 0 to 1000 to solve the problem of the map updating itself according to the desired location after a certain period of time
  • updated or deleted unnecessary codes

To test this changes run yarn prebuild and yarn android -d for Android or yarn ios -d for iOS DevBuild installations.

SVA-1365

Bild 1 Bild 2 Bild 3 Bild 4 Bild 5
Bild 6 Bild 7 Bild 8 Bild 9 Bild 10

- added `maplibre-react-native` through `yarn add maplibre-react-native`
https://github.com/maplibre/maplibre-react-native
- updated `app.json` according to `maplibre-react-native`
- added simple map to `Map.tsx` to display it in the new way

SVA-1365
- ran `yarn remove react-native-map-cluster`
- ran `yarn add supercluster`

SVA-1365
- adjusted the code accordingly to keep all old map functionalities with the new map package `'@maplibre/maplibre-react-native'`
- implemented a calculation mechanism to always show a map with multiple markers with the perfect positioning & zoom settings to display all markers on the map
- implemented new more efficient clustering via `supercluster`
- added `translucentAccent` to `colors.js` for the cluster design
- temporarily adjusted `Loadingspinner.tsx` for map design purposes

SVA-1365
@d4v3d4vE d4v3d4vE added enhancement New feature or request help wanted Extra attention is needed refactoring Updating existing code sue Sag's uns einfach labels Jul 3, 2024
@d4v3d4vE d4v3d4vE added this to the 4.0.1 milestone Jul 3, 2024
@d4v3d4vE d4v3d4vE self-assigned this Jul 3, 2024
@d4v3d4vE d4v3d4vE changed the title Refactor/sva 1365 change map package refactor(map): change map & clustering package Jul 4, 2024
app.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/components/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/components/LoadingSpinner.tsx Outdated Show resolved Hide resolved
src/components/map/Map.tsx Outdated Show resolved Hide resolved
src/components/map/Map.tsx Outdated Show resolved Hide resolved
src/components/map/Map.tsx Outdated Show resolved Hide resolved
src/components/map/Map.tsx Outdated Show resolved Hide resolved
src/components/map/Map.tsx Outdated Show resolved Hide resolved
src/components/map/Map.tsx Outdated Show resolved Hide resolved
- sorted imports
- formatted code
- removed prop `isMultipleMarkersMap`, since we calculate the exact camera positioning to show all markers on the map over the marker placements on the map to ensure always the perfect map view with all set markers
- added old cluster design with a adjustable hitbox, set to 80
- added the functionality to disable clustering, clustering is enabled by default
- added back prop functionality to all used props to ensure the same map behaviour as before
- added `View` to `LocationSettings.js` to ensure a reliable map behaviour
- added new `LoadingSinnerMap.tsx` for a compatible loading spinner overlay as the map is loading
- removed translucentAccent from `colors.js` as it's not needed anymore
- adjusted map styling all over the projects used map components to ensure a consistent map behaviour
- added `normalize()` throughout the files I worked in to ensure a consistent design

SVA-1365
@d4v3d4vE d4v3d4vE requested a review from ardasnturk July 4, 2024 16:13
- sorted styling alphabetically

SVA-1365
- sorted expo plugins

SVA-1365
package.json Outdated Show resolved Hide resolved
@d4v3d4vE d4v3d4vE requested a review from donni106 July 12, 2024 11:59
package.json Show resolved Hide resolved
- removed patchfile since it's not needed with the newest map package change

SVA-1365
@d4v3d4vE d4v3d4vE requested a review from donni106 July 18, 2024 09:24
- not needed anymore with the new map package

SVA-1365
- added old clustering behaviour, with tap on cluster it zooms in until the cluster/markers update
- updated `maximizeMapButton` based on this conversation: #1154 (comment)

SVA-1365
- adjusted `LocationSettings.js` without `react-native-collapsible` to fix the maps render error

SVA-1365
@d4v3d4vE d4v3d4vE force-pushed the refactor/SVA-1365-change-map-package branch from 11c117e to 67eb51f Compare July 18, 2024 14:13
- updated styles in `Map.tsx` to prevent the app from crashing on some screens with maps
- added `containerStyle` prop to LoadingContainer
- deleted because `LoadingSpinnerMap` was replaced with `LoadingSpinner`
- added collapsable structure in `LocationSettings` again
- added map style to `SueMapScreen` and `SueMapViewScreen` to get full screen map view
- added `onMarkerPress` and `selectedMarker` to `SueMapViewScreen` and `SueReportLocation` to show callout text when marker is pressed
- removed `setTimout`s because they caused unnecessary waiting while loading the map
- unused props removed
- added blank lines to the code to increase readability
- corrected sorting errors
- corrected some minor spelling errors

SVA-1365
alternativePosition: geoLocationToLocationObject(selectedPosition)
});
}
setSelectedPosition(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

why is this set to undefined if there is a selected position a line before?

Copy link
Member

@ardasnturk ardasnturk Jul 23, 2024

Choose a reason for hiding this comment

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

I don't know why. This code is copied and pasted into the handleSave function. The previous version of the code is protected.

Copy link
Member

Choose a reason for hiding this comment

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

@d4v3d4vE do you know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ChatGPT's explanation:

The line setSelectedPosition(undefined); is setting selectedPosition to undefined to reset or clear the selected position after the user saves or aborts the operation. This is a common practice to ensure that the state is reset, preventing any residual state from interfering with subsequent operations.

personally I couldn't explain it on the fly

Copy link
Member

Choose a reason for hiding this comment

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

so it is used first for alternative position and cleared afterwards, right? maybe the collapsible is closed with unsetting selected position? what happens if we do not set it to undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are correct. the selectedPosition is used to update the alternativePosition in the location settings and is then cleared to reset the state

the collapsible component's visibility is controlled by showMap, which is set to false immediately after clearing the selected position

but I also just checked manually various cases and I couldn't finde any error wich should appear when we are not setting setSelectedPosition(undefined);, therefore I'd say that we can get rid of it

src/helpers/index.js Outdated Show resolved Hide resolved
- added `setSelectedMarker(undefined)` to `onMapPress` function in `SueReportLocation` and `SueMapViewScreen` to remove callout text from the screen
- added `calculateBoundsToFitAllMarkers` helper to prevent the Map.tsx component from growing larger
- updated the name of the `calculateBounds` helper to `calculateBoundsToFitAllMarkers` for clarity
- added comment line to understand values in `bounds` array
- increased the `animationDuration` value from 0 to 1000 to solve the problem of the map updating itself according to the desired location after a certain period of time
- updated or deleted unnecessary codes

SVA-1365
@ardasnturk ardasnturk force-pushed the refactor/SVA-1365-change-map-package branch from 8009696 to b09cab0 Compare July 23, 2024 10:18
@donni106 donni106 assigned d4v3d4vE and unassigned d4v3d4vE Jul 23, 2024
- added `setAccessToken(null)` because the map package needs to save the access token as null to prevent the app from crashing on android devices
  - https://github.com/maplibre/maplibre-react-native/blob/main/android/install.md

SVA-1365
@ardasnturk ardasnturk force-pushed the refactor/SVA-1365-change-map-package branch from 16340a3 to d6a4a89 Compare July 23, 2024 14:48
- updated package from `alpha.5` to `alpha.10`

SVA-1365
@ardasnturk ardasnturk removed this from the 4.0.1 milestone Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed refactoring Updating existing code sue Sag's uns einfach
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants