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.

Sign up for GitHub

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

Jump to bottom

Implementation of Feature Clustering sample #823

Merged
merged 11 commits into from
Feb 9, 2024
Merged

Implementation of Feature Clustering sample #823

merged 11 commits into from
Feb 9, 2024

Conversation

mfeigl
Copy link
Collaborator

@mfeigl mfeigl commented Jan 22, 2024

Description

Implementation of the Feature Clustering Sample.

The sample works as outlined in its readme.

Checklist:

  • Set target branch to main (current release) or v.next (next release)
  • Request a reviewer
  • Assign a reviewer
  • Tag reviewer in comment

@mfeigl mfeigl requested a review from D-R-Smith January 22, 2024 09:26
@mfeigl
Copy link
Collaborator Author

mfeigl commented Jan 22, 2024

@D-R-Smith , could you please review this PR?

D-R-Smith
D-R-Smith approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@D-R-Smith D-R-Smith left a comment

Choose a reason for hiding this comment

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

@mfeigl I've made a few comments for consideration, but looks okay to me.

...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
@D-R-Smith D-R-Smith removed their assignment Jan 22, 2024
@mfeigl mfeigl requested a review from D-R-Smith February 7, 2024 13:00
@mfeigl
Copy link
Collaborator Author

mfeigl commented Feb 7, 2024

@D-R-Smith , could you do another review, please?

D-R-Smith
D-R-Smith approved these changes Feb 7, 2024
View reviewed changes
Copy link
Contributor

@D-R-Smith D-R-Smith left a comment

Choose a reason for hiding this comment

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

@mfeigl A few comments to consider but looks okay

...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
...tering/src/main/java/com/esri/samples/add_feature_clustering/AddFeatureClusteringSample.java Outdated Show resolved Hide resolved
@D-R-Smith D-R-Smith removed their assignment Feb 7, 2024
@mfeigl mfeigl requested a review from mbcoder February 7, 2024 15:44
@mfeigl
Copy link
Collaborator Author

mfeigl commented Feb 7, 2024

Assigning to @mbcoder for second review.

mbcoder
mbcoder requested changes Feb 8, 2024
View reviewed changes
Copy link
Member

@mbcoder mbcoder left a comment

Choose a reason for hiding this comment

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

Mostly looks good, but I had one question about the PopupViewer

feature_layers/add-feature-clustering/README.md Outdated Show resolved Hide resolved
@mfeigl mfeigl requested a review from mbcoder February 9, 2024 09:00
mbcoder
mbcoder approved these changes Feb 9, 2024
View reviewed changes
Copy link
Member

@mbcoder mbcoder left a comment

Choose a reason for hiding this comment

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

Looks good now,

@mfeigl mfeigl merged commit 1f44074 into v.next Feb 9, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@D-R-Smith D-R-Smith D-R-Smith approved these changes

@mbcoder mbcoder mbcoder approved these changes

Assignees

@mbcoder mbcoder

@mfeigl mfeigl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mfeigl @mbcoder @D-R-Smith