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

Added optional parameter to the aui-leaflet-locate-control to specify the zoomlevel #106

Merged
merged 13 commits into from
Mar 11, 2019

Conversation

Matthias-Claes
Copy link
Contributor

The parameter is optional and if no value is given it will take the value that was previously used as the default value

PR Checklist

This PR fulfills the following requirements:

  • The commit message follows our guidelines: Contributing guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • A changelog entry was added

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

The 'aui-leaflet-locate-control' has a set zoomlevel it will always use.

Issue Number: N/A

What is the new behavior?

The 'aui-leaflet-locate-control' now has a 'zoomlevel' parameter for users to specify.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Resolved issues

… the zoomlevel

The parameter is optional and if no value is given it will take the value that was previously used as the default value
@Matthias-Claes
Copy link
Contributor Author

Matthias-Claes commented Jan 25, 2019

@jsebrech @TriangleJuice @lievenvg @TomVanDenBroeck

Ligt het aan een fout langs mijn kant dat de build failt? Het lijkt er niet op dat ik changes heb gemaakt waardoor de build zou kunnen failen?

@lievenvg
Copy link
Contributor

lievenvg commented Jan 25, 2019

Ligt het aan een fout langs mijn kant dat de build failt? Het lijkt er niet op dat ik changes heb gemaakt waardoor de build zou kunnen failen?

@Matthias-Claes
Dit is een falende unit test die niet overweg kan met de maand januari.
In PR #105 werd deze unit test gefixt. Dus zodra die PR goedgekeurd en gemerged is, kan je die code in jouw branch binnen halen.

Copy link
Contributor

@lievenvg lievenvg left a comment

Choose a reason for hiding this comment

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

Don't forget to update the map unit tests as well.

CHANGELOG.md Outdated Show resolved Hide resolved
packages/map/examples/src/map/pages/demo/demo.page.html Outdated Show resolved Hide resolved
packages/map/lib/README.md Outdated Show resolved Hide resolved
@@ -119,13 +119,13 @@ export class LeafletMap {
}

// CENTERING
locate() {
locate(zoomlevel: number = 19) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to do a check if the given zoom level is a valid value.
And if you're going to set the default value here as well, it's better to set it in a shared config file.

Copy link
Contributor

@TriangleJuice TriangleJuice left a comment

Choose a reason for hiding this comment

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

On top of our inline comments, I would suggest using ‘zoom level’ (in text) and zoomLevel (In code), since ‘zoomlevel’ isn’t correct English.

Copy link
Contributor

@lievenvg lievenvg left a comment

Choose a reason for hiding this comment

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

If u can just update the public remark.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lievenvg lievenvg left a comment

Choose a reason for hiding this comment

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

Overlooked the merge conflict, sorry..

Copy link
Contributor

@TriangleJuice TriangleJuice left a comment

Choose a reason for hiding this comment

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

Code is perfect like this! Is it possible to list the default zoomLevel in the readme, or even add a small link to the Leaflet documentation on what this zoom level actually means. Thanks!

@lievenvg lievenvg merged commit 2279a2a into digipolisantwerp:master Mar 11, 2019
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