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

Issue #64 Add Marker/Pointer Inspect Widget to support Titiler COG layers #81

Merged
merged 15 commits into from
Aug 2, 2023

Conversation

sandrahoang686
Copy link
Collaborator

@sandrahoang686 sandrahoang686 commented Jul 28, 2023

Summary:

  • What did I do?

  • This adds the inspect/pointer widget feature.

  • This feature only currently works for /cog layers from titiler

  • Why *** did I do it? (What was the motivation?)

  • How *** can someone test or demo my changes?

    *** When relevant, use screenshots to document
    One Layer
    Screenshot 2023-07-31 at 1 56 42 PM

Multiple Layers
Screenshot 2023-07-31 at 1 58 06 PM

No Markers
Screenshot 2023-07-31 at 1 58 31 PM

No Layer applied
Screenshot 2023-07-31 at 1 59 59 PM

/mosaics layer applied (will behave as if there is no layer currently)
Screenshot 2023-07-31 at 2 01 23 PM

Fixes or Addresses Issue #: [ticket number & link]

#64

Checklist before requesting a review:

  • My code follows the guidelines of this project
  • I performed a self-review of my code changes
  • I have completed spell-checking, removed unnecessary print statements & commented-out code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes

@sandrahoang686 sandrahoang686 marked this pull request as ready for review July 31, 2023 17:50
@sandrahoang686 sandrahoang686 changed the title Add inspect widget and display Issue #64 Add Marker/Pointer Inspect Widget to support Titiler COG layers Jul 31, 2023
@sandrahoang686 sandrahoang686 requested a review from emmalu July 31, 2023 18:19
Copy link
Collaborator

@emmalu emmalu left a comment

Choose a reason for hiding this comment

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

I think it's looking quite good! Nice extension of what's already there. Let me know if you'd like to chat about any of my comments.

def handle_interaction(self, action, geo_json, **kwargs):
def handle_clear(event):
draw_layer = main.find_layer("draw_layer")
main.remove_layer(draw_layer)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably clear the Polygon coordinates here too - otherwise, we have ghost coordinates (values, but not indication of what they belong to because the polygon on the map has disappeared.

Screenshot 2023-07-31 at 8 32 10 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

One additional thought is that it might be better to pull the clear button out from the tabs and handle its state, trigger it, every time there is a switch between polygon and marker.

Screenshot 2023-07-31 at 8 32 57 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ghost coordinates taken care of 👍🏼 as for the additional thought, its a great one. Logic is mostly duplicated here and switching between tabs and if there is a layer with values, we should trigger on switch. I made a TODO note in the code to tackle for later as there are other TODOs in which code can be reorganized

if layers_data:
display_data(layers_data)
else:
point_data.value = f"<p><b>Coordinates:</b></p><code>{self.coordinates}</code><br/>"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any thoughts on handling in the same html_template pattern as the rest of the data? Not a huge deal, but just an idea to keep things in a similar style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh yes, I think this is good hear because display_data is a for loop that loops over all the visibile applied layers! But I guess the function names are not very explicit about that so let me change the names 🏃🏼‍♀️

Comment on lines 178 to 183
if self.draw_control_added:
self.remove(self.draw_control)
self.draw_control_added = False
if self.inspect_control_added:
self.remove(self.inspect_control)
self.inspect_control_added = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we combine these? I don't think it's possible to view one without the other.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm not too sure what you mean here, this is related the pointer or square area drawer control. When one tab Point or Area is selected and if a control was already applied, it needs to be removed, not sure if this is related to what you are asking though

@sandrahoang686 sandrahoang686 merged commit 4057dc9 into main Aug 2, 2023
@wildintellect wildintellect deleted the feature/add-marker-inpect branch September 27, 2023 22:14
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.

2 participants