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

Replace tooltip with side panel #636

Merged
merged 3 commits into from
Oct 2, 2024
Merged

Replace tooltip with side panel #636

merged 3 commits into from
Oct 2, 2024

Conversation

vgeorge
Copy link
Member

@vgeorge vgeorge commented Sep 18, 2024

This addresses #586 by adding a scrollbar to the tooltip when there is a long list of properties. Work in progress.

We discussed adding a side panel for browsing the properties, but I wanted to explore an alternative that enhances the existing tooltip rather than introducing a new UI element.

In this implementation, the getTooltip property in Deck.gl has been replaced with a custom tooltip rendered separately. This change was necessary because the trigger for displaying the tooltip was modified from mouse hover to mouse click. As a result, the tooltip persists when the mouse pointer moves away, allowing the user to scroll through the available properties.

Known Issues:

  • This is based off release 0.10.0-beta.2 commit, the main branch appears to be broken due to dependencies updates
  • A horizontal scrollbar is always displayed alongside the vertical one due to an issue determining the table width, this needs an CSS update
  • The tooltip remains in the same position when the map is panned. We could either hide it or have it move along with the map
  • We are moving away from a Deck.gl-based solution. It might be worth implementing this as a custom widget, but the complexity of doing so is unclear at this point

How to test

Open examples north-america-roads and internet-speeds and click on a rendered feature. The popup should be displayed and the table should be scrollable when the list is long.

cc @kylebarron @batpad

@kylebarron
Copy link
Member

kylebarron commented Sep 19, 2024

Looks fine at a glance.

I'd suggest including screenshots or screencasts in PRs. It makes it easier to consider the changes without pulling this branch, rebuilding, bringing up an example notebook, and running it. It also preserves the semantic meaning of this PR for us to look back at in the future.

  • This is based off release 0.10.0-beta.2 commit, the main branch appears to be broken due to dependencies updates

Maybe that's from #629. Would you be able to try downgrading and see if it fixes your issue?

  • The tooltip remains in the same position when the map is panned. We could either hide it or have it move along with the map

Remains in the same pixel position or in the same geospatial position?

@vgeorge
Copy link
Member Author

vgeorge commented Sep 24, 2024

@kylebarron and I had a chat about this. Some notes:

  • Having a side panel would be better for inspecting the data, as it won't cover nearby features like the tooltip does. We'll shift from a tooltip to a scrollable sidebar
  • With the removal of the tooltip there would be no indication of the highlighted feature and Kyle suggested looking into the autoHighlight Layer property

cc @batpad

@vgeorge vgeorge marked this pull request as ready for review October 2, 2024 11:54
@vgeorge
Copy link
Member Author

vgeorge commented Oct 2, 2024

Changes Added

  • Merged the latest commit from the main branch, fixing the dependency issue
  • Replaced the tooltip with a side panel

Current Behavior

  • Clicking on a feature on the map opens a left-side panel, adjusting the map width accordingly
  • A button is available to close the panel
  • A scrollbar is displayed if necessary
  • BBox selection works regardless of whether the panel is open or closed

Internet Speeds Example

internet-speeds

North American Roads Example

roads

Feature Highlight

After setting auto_highlight=True, I observed the following behavior:

autohighlight

While light features are distinguishable, darker ones are not, so I’m unsure if this is the best approach.

We can explore changing the mouse pointer style to pointer to better indicate that it is possible to click on a feature. However, this is not as straightforward as simply enabling auto highlight, so we might need to address this separately if we want to include the side panel feature in the next release.

@kylebarron please let me know your toughts.

@kylebarron kylebarron requested a review from batpad October 2, 2024 14:32
@kylebarron
Copy link
Member

This looks amazing!!

After setting auto_highlight=True, I observed the following behavior:

While light features are distinguishable, darker ones are not, so I’m unsure if this is the best approach.

I don't think this is a problem because the end user can set a custom highlightColor: https://deck.gl/docs/api-reference/core/layer#highlightcolor

(Well, it looks like we need to actually expose that through Lonboard)

We can explore changing the mouse pointer style to pointer to better indicate that it is possible to click on a feature.

In terms of optimal UI, it seems like it would be great to switch between "normal cursor" and "pointer cursor" depending on whether there's something active under the cursor to click on. However since deck.gl is in a WebGL canvas, I'm almost certain that's not possible.

However, this is not as straightforward as simply enabling auto highlight, so we might need to address this separately if we want to include the side panel feature in the next release.

I think it's fine to just document new behavior that you need to click instead of just highlight on objects. I don't want to enable auto highlight by default because it doesn't work properly on multi-geometries.

@vgeorge vgeorge changed the title Add scroll to tooltip Replace tooltip with side panel Oct 2, 2024
@vgeorge
Copy link
Member Author

vgeorge commented Oct 2, 2024

@kylebarron thanks for the review! I updated the PR title to better describe what has changed. I'll repost your comments on the upstream ticket to follow up the discussion.

@vgeorge vgeorge merged commit 44a45f8 into main Oct 2, 2024
5 checks passed
@vgeorge vgeorge deleted the enhance/tooltip branch October 2, 2024 16:24
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