Skip to content

Feature/map pin cluster#51

Open
LProulx86 wants to merge 11 commits intomasterfrom
feature/MapPinCluster
Open

Feature/map pin cluster#51
LProulx86 wants to merge 11 commits intomasterfrom
feature/MapPinCluster

Conversation

@LProulx86
Copy link
Contributor

GitHub Issue: #

Proposed Changes

This is adding Clustering option to Cartography.

Add Clustering to cartography

Checklist

Please check if your PR fulfills the following requirements:

  • Documentation has been added/updated
  • Automated Unit / Integration tests for the changes have been added/updated
  • Contains NO breaking changes
  • Updated the Changelog
  • Associated with an issue

Other information

@LProulx86 LProulx86 requested a review from carlh98 May 12, 2023 17:38
@carlh98 carlh98 requested review from a team, Guidemarcus and lamonfly May 12, 2023 17:44
@MatFillion
Copy link
Member

changelog should be updated :)

@LProulx86 LProulx86 closed this May 12, 2023
@LProulx86 LProulx86 reopened this May 12, 2023
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this setter be private?

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be called IsClusteringEnabled? Using "Cluster" suggests that there is only 1 cluster.

Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? It seems like the xaml associated with this VM wasn't modified?

Copy link
Member

Choose a reason for hiding this comment

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

Also the same comments from the DynamicMap_FeaturesPageViewModel apply here too.

Copy link
Member

Choose a reason for hiding this comment

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

I know this comes from a previous PR, but why is the DynamicMap package referencing Google maps on iOS? Shouldn't there be a package specific for GoogleMaps on iOS? GoogleMaps is not the default maps provider on iOS.

Copy link
Member

Choose a reason for hiding this comment

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

The xml doc on the interface itself should be updated too.
And you could fix the grammar at the same time instead of just adding the period.

Copy link
Member

Choose a reason for hiding this comment

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

  • managed
  • will handle handles

Copy link
Member

Choose a reason for hiding this comment

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

will be manage are managed

Copy link
Member

Choose a reason for hiding this comment

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

This method (and the one below) should be prefixed by "Get".
Any method should start with a verb.

Copy link
Member

Choose a reason for hiding this comment

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

Changing to file scoped namespaces changes all the lines of the file, making it almost impossible to spot the changes that are actually related to clustering. I would suggest you undo that change and do it in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Where does the -10 value come from?

CHANGELOG.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I'm nit-picky but use some capital letters please. Don't do it if you don't want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would like some documentation please.

@jeanplevesque
Copy link
Member

jeanplevesque commented May 16, 2023

Add screenshots and/or gifs of the results from the samples in the PR description.

@carlh98 carlh98 force-pushed the feature/MapPinCluster branch 2 times, most recently from 3d4e6d3 to 65dfa01 Compare July 6, 2023 15:16
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.

6 participants