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

Deprecate obsolete fit parameter #1368

Merged

Conversation

Robbendebiene
Copy link
Contributor

@Robbendebiene Robbendebiene commented Sep 19, 2022

I always wondered what the use of the zoom parameter of the FitBoundsOptions class was.
When working on #1367 I finally had the chance to look into it. As it turns out this parameter is never used nor is it documented.

Since this is a somewhat breaking change I made a separate PR.

@JaffaKetchup
Copy link
Member

(dammit not another one :D)

We'll get round to looking at this, please don't take the silence badly :)

@Robbendebiene
Copy link
Contributor Author

I knew that this PR wouldn't be welcomed with joy. 😅

Since this has no importance at all it would probably make sense to delay this PR till another more relevant breaking change kicks in. But maybe it also turns out that the removed property has an actual usage.

@JaffaKetchup
Copy link
Member

Nah, I was more annoyed that someone found yet another 'remnant of the old world' as I call them!
Where did it come from? Where does it go? Nobody knows :D

Thanks for the PR, they are always appreciated. As this literally did nothing, no one should be using it (or if they are, they shouldn't care that it's gone), and therefore I wouldn't call it a breaking change, just a minor change (v0.x.0).

I'm quite busy atm, so I'll see what I can get done when :)

@TesteurManiak
Copy link
Collaborator

I have a suggestion, as this is a "breaking change" even if the parameter is not used, it could be considered to deprecate it and force to use MapOptions.zoom instead, also adding a comment as a reminder to remove this property for the next major release.
By doing so the PR can be merged immediately and there won't be breaking change until further changes.

class FitBoundsOptions {
  final EdgeInsets padding;
  final double maxZoom;
  final bool inside;

  const FitBoundsOptions({
    this.padding = EdgeInsets.zero,
    this.maxZoom = 17.0,
    @Deprecated('This property is never used, use MapOptions.zoom instead.')
        double? zoom,
    this.inside = false,
  });
}

@JaffaKetchup
Copy link
Member

Sure, that seems sensible.

@Robbendebiene
Copy link
Contributor Author

Sounds good to me. The only question I have is, should we really point to the MapOptions.zoom property? As far as I know this is just setting the default/initial zoom value. Honestly I have no clue what this property was meant to do, but I doubt that it was forcing any zoom value since this would break the functionality of the fit bounds method.

@TesteurManiak
Copy link
Collaborator

Then I guess you can just put: This property is unused and will be removed in the next major release. 🤔

@Robbendebiene
Copy link
Contributor Author

I've added two deprecation notices so we get:

grafik

@Robbendebiene Robbendebiene changed the title Remove obsolete fit parameter Deprecate obsolete fit parameter Sep 22, 2022
Copy link
Collaborator

@TesteurManiak TesteurManiak left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@JaffaKetchup JaffaKetchup left a comment

Choose a reason for hiding this comment

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

Approving on behalf of @TesteurManiak.

@JaffaKetchup JaffaKetchup merged commit 923cfc6 into fleaflet:master Sep 22, 2022
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