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

Render the draw vertex to be open to prevent occluding the target point. #2241

Merged
merged 8 commits into from
Feb 23, 2024

Conversation

sufyanAbbasi
Copy link
Contributor

Fixes #2110

Sets the draw tool to a static color and sets the draw target to be an open circle, allowing for better view of the data underneath.

  • Redraws the draw vertex to be an open circle, which will allow users to see the point underneath.
  • Renders a small bull's eye at the selected point (due to technical limitations with the line string).
  • Sets a static color for the entire draw tool.
  • Implement the distance calculation (out of scope?).
  • Tests pass.
  • Created a survey with a polygon selection job.
  • Verified that the draw tool is rerendered and produces valid geometries.

Screenshot 2024-02-14 at 8 49 03 PM

Screenshot 2024-02-14 at 8 49 41 PM

@gino-m

@sufyanAbbasi sufyanAbbasi requested a review from gino-m February 15, 2024 05:02
<path
android:pathData="M12,12m-10.5,0a10.5,10.5 0,1 1,21 0a10.5,10.5 0,1 1,-21 0"
android:strokeWidth="0"
android:fillColor="@color/drawAreaLineStringStrokeColor"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry to ask, but we just finalized the designs here:

image

Any chance you could update the styling to match? Just asked @rawbzz to confirm whether the color should match the polygon (job color) or be fixed. Thanks for your patience!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! No worries, happy to refactor!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And I'm assuming that we'll be using the white circle to indicate the vertices, including the endcap, but the plus-sign is going to move independently on its own when panning, to minimize occlusion of the base layer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL!

Screenshot 2024-02-15 at 5 52 17 PM
Screenshot 2024-02-15 at 5 52 30 PM (3)
Screenshot 2024-02-15 at 5 52 49 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I'm assuming that we'll be using the white circle to indicate the vertices, including the endcap, but the plus-sign is going to move independently on its own when panning, to minimize occlusion of the base layer?

Hey I hadn't thought of that, but it's a great idea. The only this is that this will only work because of a bug / incomplete FR where the edge only snaps to the map center when the camera stops moving. But the way you've done it makes this a feature rather than a bug, so LGTM. @rawbzz FYI

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

A few suggestions. Thanks!

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

P.S. Run Gradle target ktfmtFormat to fix formatting issues.

Copy link
Collaborator

@gino-m gino-m left a comment

Choose a reason for hiding this comment

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

⭕ LGTM!

@shobhitagarwal1612
Copy link
Member

> Task :workspace:detekt FAILED
/workspace/ground/src/main/java/com/google/android/ground/ui/util/ColorUtil.kt:25:5: Functions with exact one statement, the return statement, can be rewritten with ExpressionBodySyntax. [ExpressionBodySyntax]

Some code quality checks are still failing. You can run these checks locally via gradle target checkCode

@sufyanAbbasi
Copy link
Contributor Author

> Task :workspace:detekt FAILED
/workspace/ground/src/main/java/com/google/android/ground/ui/util/ColorUtil.kt:25:5: Functions with exact one statement, the return statement, can be rewritten with ExpressionBodySyntax. [ExpressionBodySyntax]

Some code quality checks are still failing. You can run these checks locally via gradle target checkCode

Thank you! Turns out the whole file can be deleted, as it is now unused. Done.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 52.42%. Comparing base (c5a130e) to head (f1952f9).

Files Patch % Lines
...d/ground/ui/map/gms/features/LineStringRenderer.kt 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #2241   +/-   ##
=========================================
  Coverage     52.42%   52.42%           
  Complexity     1222     1222           
=========================================
  Files           317      317           
  Lines          6264     6264           
  Branches        659      659           
=========================================
  Hits           3284     3284           
  Misses         2617     2617           
  Partials        363      363           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shobhitagarwal1612 shobhitagarwal1612 merged commit ec64292 into master Feb 23, 2024
4 checks passed
@shobhitagarwal1612 shobhitagarwal1612 deleted the sufy/2110/draw-vertex branch February 23, 2024 04:48
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.

[Draw an area] Improve vertex selector UX treatment
4 participants