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

Support for flutter_map version 4.0.0 #34

Merged
merged 8 commits into from
May 10, 2023
Merged

Conversation

josxha
Copy link
Contributor

@josxha josxha commented May 6, 2023

This pull requests adds support for new new major version of flutter_map by implementing the changes suggested in fleaflet/flutter_map#1455 (comment).

Additional changes:

  • remove allowPanningOnScrollingParent: false instruction from README.md
  • migrate example project
  • add "List Page" to the example project to test behaviour in a ListView (I can remove it in case you don't like)
  • fix lints
  • raise min sdk version to use super.key in the constructor of DragMarkers
  • loosen dependency constriant to support flutter_map 3 and 4.

I tested the pull request on my android device where it worked well.

@ibrierley
Copy link
Owner

Thanks for this, really appreciate it, as I haven't had much time of late. Will try and just get it checked over the next day or two.

Copy link
Collaborator

@pablojimpas pablojimpas left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!
I've tested this on Android, Linux, and the Web and works fine.

I've noticed that the code is not fully formatted, can you please run:

  • dart format lib
  • dart format example/lib

Can you please address my other review comments as well?

Thank you again.

pubspec.yaml Outdated Show resolved Hide resolved
example/pubspec.yaml Outdated Show resolved Hide resolved
example/lib/list_page.dart Outdated Show resolved Hide resolved
@ibrierley
Copy link
Owner

Just tested, I'm fine with all of this thanks ! Are you ok to make the suggested changes ?

Copy link
Collaborator

@pablojimpas pablojimpas left a comment

Choose a reason for hiding this comment

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

Everything looks good to me.
Thanks, @josxha!

@josxha
Copy link
Contributor Author

josxha commented May 8, 2023

Thanks a lot for the code review and testing @pablojimpas!
I added your requested changes. 👍

I would also like to suggest to add a selection of platforms integrations to the example project or alternatively to add these directories to git ignore.

Arguments for a push:

  • Easier for interested people to test the project on their own devices .
  • New contributors don't have to create their own platform integration to test changes.
  • Example projects don't appear in the unstated changes

Arguments for a gitignore :

  • native integrations increase the maintenance effort
  • clear and minimalistic repository
  • Example projects don't appear in the unstated changes

@pablojimpas
Copy link
Collaborator

I would also like to suggest to add a selection of platforms integrations to the example project or alternatively to add these directories to git ignore.

Most Flutter packages include a full example app with all (or at least some) platform-specific projects in their repository. I think it's a good practice to have both a full example app and a small example (as minimal as possible) showcasing how to use the package in a way that can be easily copy-pasted into the users' app.

My general idea will look like this:

example
├── demo_app
│   ├── analysis_options.yaml
│   ├── android
│   ├── ios
│   ├── lib
│   ├── linux
│   ├── macos
│   ├── pubspec.lock
│   ├── pubspec.yaml
│   ├── README.md
│   ├── web
│   └── windows
├── main.dart
└── README.md

With this approach, the contents of example/main.dart will appear in the example tab for everyone to copy, and then we will maintain more detailed and complex examples under the “full example” demo app.

I will prepare a PR for this suggestion if @ibrierley agrees.
Furthermore, I would like to make a PR for flutter_map_dragmarker syncing the changes made here. And then, another PR to this repo with the aim of using dragmarker.dart as a pub dependency rather than including the file directly into this repo and maintaining it in two different places.

@ibrierley
Copy link
Owner

Heya, I'm a bit unsure what the extra stuff brings, do you mean you can just install the app, rather than running it from your IDE or something ? (I'm only really familiar with the normal IDE integration of Android Studio etc). I don't have any issue with extra files generally, but if it's more maintenance, then it's more of an issue (I don't have a mass of time atm so a bit wary, but I'm also happy to add any collaborators or whatever for others to be able to do more !).

For the dragmarker sync, I'm fine with that, but double check it all, as iirc there was some issue where I ended up making the line editor not dependent on dragmarker plugin. It was a long while ago though, I think it was related to the retaining of state with different dragmarkers. I have a feeling this is fixed potentially after I added Keys to dragmarker, just you may find you need to pass a Key through to each Dragmarker or something similar (may have been related to something like this PR

@pablojimpas
Copy link
Collaborator

Heya, I'm a bit unsure what the extra stuff brings, do you mean you can just install the app, rather than running it from your IDE or something ? (I'm only really familiar with the normal IDE integration of Android Studio etc). I don't have any issue with extra files generally, but if it's more maintenance, then it's more of an issue (I don't have a mass of time atm so a bit wary, but I'm also happy to add any collaborators or whatever for others to be able to do more !).

Having a folder for each specific platform allows anyone to just execute flutter run to launch the example app instead of recreating the project first with flutter create . and overall brings a faster development cycle for contributors since they can test their changes directly. I doubt that it creates a lot of maintenance effort, since those platform-specific files are only created once and rarely touched again (unless you're actually building a real production app).

I am happy to help you and become a maintainer of both this package and flutter_map_dragmarker since I'm relying on them heavily in my app, so I want to see them thrive. Talk to me privately on Discord or elsewhere if you find this appealing.

For the dragmarker sync, I'm fine with that, but double check it all, as iirc there was some issue where I ended up making the line editor not dependent on dragmarker plugin. It was a long while ago though, I think it was related to the retaining of state with different dragmarkers. I have a feeling this is fixed potentially after I added Keys to dragmarker, just you may find you need to pass a Key through to each Dragmarker or something similar (may have been related to something like this PR

Got it! I will double-check the history of both repos to make sure that I don't fall into the same issue.

@ibrierley
Copy link
Owner

Ok, that all sounds great. Is this waiting on anything else now, otherwise I can test and merge. (I'm guessing the rest will be part of a separate PR). I'll ping you via email (happy to chat on Discord if easier as you mention), or if anyone else is interested as well, let me know.

@pablojimpas
Copy link
Collaborator

Yes @ibrierley, the other stuff will be part of future PRs, this one is ready as is. You are welcome to give it a last test and merge it.

@ibrierley
Copy link
Owner

Just thinking, won't we ideally want to change the version of line_editor now, eg to 5.0 ? Then it will be easier for some to resolve issues when it gets published ?

@josxha
Copy link
Contributor Author

josxha commented May 9, 2023

Just thinking, won't we ideally want to change the version of line_editor now, eg to 5.0 ? Then it will be easier for some to resolve issues when it gets published ?

I don't think that a major release is necessary because of it's backwards compatibility. A minor release should be enough since there are no breaking changes?

...btw is there any discussion going on in Discord? May I join the discussion?

@ibrierley
Copy link
Owner

Good question actually, I'm a little unclear on dependency changes. We need it to update, for pub.dev new version I think...as to whether its a major or not...it's probably fine without, as the major changes are with flutter_map alone which has updated its major...

There's no discussion currently on Discord, I'm happy to discuss anything there, or I typically use email more often (can use my github username @gmail.com if anyone ever needs), or ianb on flutter_map.

@ibrierley ibrierley merged commit 947ef1c into ibrierley:master May 10, 2023
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.

3 participants