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

Feat/new map solution #220

Merged
merged 8 commits into from
Sep 21, 2024
Merged

Feat/new map solution #220

merged 8 commits into from
Sep 21, 2024

Conversation

simon-the-shark
Copy link
Member

So I did try to use flutter map package and imo it's pretty lightweight. Also it's kinda like building blocks so we can add more features later and we have very big control over what's happening there. Drawbacks to this is e.g. that we have to implement my location button ourselves.

So open street map doesn't look as cool as other providers, but is 100% free, pretty functional and if we want to use different style we can try to selfhost it at some point in the future.

potencial TODOs for the future:

tell me what you think :)
image

@simon-the-shark simon-the-shark added the enhancement New feature or request label Sep 20, 2024
@simon-the-shark simon-the-shark added this to the v1.0.0 milestone Sep 20, 2024
@simon-the-shark simon-the-shark self-assigned this Sep 20, 2024
Copy link
Member

@mikolaj-jalocha mikolaj-jalocha left a comment

Choose a reason for hiding this comment

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

Good work! Substantial amount of code here've been refactored. Few notes below:

  • In my opinion OSM design looks quite nice. It is very readable and for me - suits even better than previous one. But interesting and challenging task for the future (regarding creating our own design)'d nice ;)
  • At least on emulator performance's been significantly improved
  • I think there's need for good looking animation under the condition that it wouldn't reduce performance.
  • On Android device I couldn't see the compass
  • Below I posted some of my suggestion (minor ones). Treat them as suggestions, not required changes of any kind.
  • Also, I've posted some questions of mine regarding current solutions
  • I've created some tasks that you mentioned. Please convert them to issues, once pr's resolved ;) Currently I didn't include markers clustering, idk what are the pros and cons here. Explanation needed 😂

lib/features/buildings_view/model/building_model.dart Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Can the old animations be placed here? (The ones before changes)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah but we need separate package for this to be able to animate camera, and the way it's structured, we need to do some refactor with this too: https://pub.dev/packages/flutter_map_animations

lib/features/map_view/widgets/map_atrribution.dart Outdated Show resolved Hide resolved
lib/features/map_view/widgets/map_atrribution.dart Outdated Show resolved Hide resolved
lib/features/map_view/widgets/map_widget.dart Outdated Show resolved Hide resolved
@@ -29,7 +29,7 @@ class _NoTransitionRoute extends CustomRoute {
}

@AutoRouterConfig(replaceInRouteName: "View,Route")
class AppRouter extends _$AppRouter {
class AppRouter extends RootStackRouter {
Copy link
Member

Choose a reason for hiding this comment

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

Why the change here? Just curious

Copy link
Member Author

Choose a reason for hiding this comment

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

it's caused by a new version of auto_route

ref.read(parkingsMapControllerProvider.notifier).onMarkerTap(item);
},
alignment: Alignment.topCenter,
child: GestureDetector(
Copy link
Member

Choose a reason for hiding this comment

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

Looks nice. Did you think about some splash effect here? Could improve UX

Copy link
Member Author

Choose a reason for hiding this comment

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

I like the idea

Copy link
Member Author

Choose a reason for hiding this comment

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

but not sure how this would look, opening up a task for the future reference

Copy link
Member

Choose a reason for hiding this comment

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

Can compass be seen on Android?

Copy link
Member

Choose a reason for hiding this comment

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

I can't see that 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

it's disabled when the map is straight to the north. It should pop up when you rotate the map. It can be configured otherwhise too

@simon-the-shark simon-the-shark merged commit 368820d into main Sep 21, 2024
2 checks passed
@simon-the-shark simon-the-shark deleted the feat/new-map-solution branch September 21, 2024 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants