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

Minor documentation updates #1377

Closed
wants to merge 2 commits into from
Closed

Minor documentation updates #1377

wants to merge 2 commits into from

Conversation

danbits
Copy link

@danbits danbits commented Sep 26, 2022

When setting up flutter_map with the vector_map_tiles plugin, I noticed a couple of things to update in the docs.

  • The basic flutter_map usage example uses a named argument controller. I needed to update the argument to mapController in order to get the example working.
  • The section on vector tiles mentions the "mixed" mode for vector_map_tiles, which has been removed. It looks like the paragraph was removed in update vector_map_tiles docs #1361, but that change did not make its way into 1dfdc38. Sorry to bring this up again if that was the intention.

Thank you for all of the work on this package. It's been a joy to use!

This feature has been removed from `vector_map_tiles` as it lacked
obvious performance benefits. For more information, see:

* greensopinion/flutter-vector-map-tiles#31
* greensopinion/flutter-vector-map-tiles#52
@JaffaKetchup
Copy link
Member

Hi @danbits. Number 1 seems a bit odd to me, can you show some of your code please? And you are correct about number 2, thanks for pointing that out.

@TesteurManiak
Copy link
Collaborator

He's right the parameter name has been changed:

final MapController? mapController;

@JaffaKetchup
Copy link
Member

Ah. I'm not sure this is supposed to be like this? I can't remember it being that way in the v3 release? If it wasn't, that needs reverting I think.

@TesteurManiak
Copy link
Collaborator

Just checked, the last occurence of controller was in #1289 then it became mapController in #1294

@danbits
Copy link
Author

danbits commented Sep 26, 2022

Hey, thanks for the quick responses!

I'm assuming that @TesteurManiak's explanation is sufficient, so let me know if you still want to see any code!

@TesteurManiak
Copy link
Collaborator

Well, the thing is that apparently the name mapController is used since the version 2.1 of the package, so changing it back to controller will have an impact on all users who have migrated to 3.0.
I would recommend to deprecate mapController and add back the controller parameter to avoid any breaking changes 🤔

@ibrierley
Copy link
Collaborator

Does it really matter, if it was part of a breaking change already ? I.e I would guess most of the old examples out in the wild are already incorrect with other changes anyway (eg layer changes to children), so it feels like simplest would just be to go with it and update the docs to match, or am I missing something (quite likely!) ?

@TesteurManiak
Copy link
Collaborator

My main concern is that changing it back to controller in the next release would break project that are using version 3.0.
Usually, when updating code implies breaking change you want to make it a major version bump to avoid current users to encounter a regression in their codebase. The change is small but still, I'm not a big fan of breaking the code of current users of the package.
This might seem a bit overkill, but imo it's better to deprecate anything that can be deprecated before introducing breaking changes.

@ibrierley
Copy link
Collaborator

ibrierley commented Sep 26, 2022

Just checking, are we talking about the same thing here...hasn't it actually been mapController since the dawn of time...? Where has it ever been controller ?

@ibrierley
Copy link
Collaborator

Ah, maybe there's a bit of confusion with MapOptions vs FlutterMap mapController (not surprising if so!)

@TesteurManiak
Copy link
Collaborator

You're right, I'm the one who inverted controller and mapController 🤣
So yeah there's no problem actually, just updating the documentation to be cohesive with the code

@TesteurManiak
Copy link
Collaborator

So yeah, I guess it can be merged (sorry again for my mistake it has been a long day 😅 )

@ibrierley
Copy link
Collaborator

Hah no worries, this gets most of us at some point, and one of the reasons we've tried to simplify stuff :D.

Cue someone out there getting confused about bounds next :D

@JaffaKetchup
Copy link
Member

Ok, so if we're happy with this, I'll make the changes manually ASAP, as it's just a small changeset. Haven't been following closely (been busy), so please just confirm that's the case :)

@ibrierley
Copy link
Collaborator

Fine with me

@JaffaKetchup
Copy link
Member

Great. Will add those changes ASAP. Thanks for the heads up!

gitbook-com bot pushed a commit that referenced this pull request Sep 26, 2022
@danbits danbits deleted the doc_updates branch September 27, 2022 00:26
@@ -4,7 +4,7 @@

```dart
FlutterMap(
controller: MapController(),
mapController: MapController(),
Copy link
Author

Choose a reason for hiding this comment

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

@JaffaKetchup Should this line be included as well?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Sorry, missed this one!

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.

4 participants