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

Cleanup popup #6207

Merged
merged 8 commits into from
May 7, 2024
Merged

Cleanup popup #6207

merged 8 commits into from
May 7, 2024

Conversation

ahuang11
Copy link
Collaborator

@ahuang11 ahuang11 commented Apr 23, 2024

Changes so that internally, it will not touch existing_pane/popup_pane's param. That is up to the developer/user, so if it's not visible, the popup pane will not show, primarily for holoviz/holonote#101

@ahuang11 ahuang11 added the type: enhancement Minor feature or improvement to an existing feature label Apr 23, 2024
Copy link

codecov bot commented Apr 23, 2024

Codecov Report

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

Project coverage is 87.57%. Comparing base (eb7ae9d) to head (40d59e0).
Report is 4 commits behind head on main.

Files Patch % Lines
holoviews/plotting/bokeh/callbacks.py 0.00% 4 Missing ⚠️
holoviews/tests/ui/bokeh/test_callback.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6207      +/-   ##
==========================================
- Coverage   88.12%   87.57%   -0.56%     
==========================================
  Files         319      321       +2     
  Lines       67221    67289      +68     
==========================================
- Hits        59241    58929     -312     
- Misses       7980     8360     +380     

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

@ahuang11
Copy link
Collaborator Author

I believe these changes make the experience better:

  • popup_pane visibility is controlled by the user; if it's visible=False then popup pane won't show
  • Setting a popup_pane with tooltips to have a position to NaN resulted in the tooltips still appearing at the corner of the page (notice it the top right). Changing it to visible = false makes that not happen.
image

The only thing I couldn't figure out how to do is de-duplicate the same layout across multiple streams, e.g. TapStream and BoxSelect both popup _layout. Not sure how to check if the ID of this layout already exists in the document; if so, delete the previous one.

image

@ahuang11 ahuang11 requested a review from hoxbro May 6, 2024 15:07
Co-authored-by: Simon Høxbro Hansen <[email protected]>
@ahuang11 ahuang11 requested a review from hoxbro May 7, 2024 15:32
@hoxbro
Copy link
Member

hoxbro commented May 7, 2024

I can't judge if this is a better approach than the previous approach.

If your opinion is that it is, you can merge the PR unless @philippjfr has a hard objection.

@philippjfr
Copy link
Member

I'm happy with this approach.

@hoxbro hoxbro merged commit 85564f1 into main May 7, 2024
12 of 13 checks passed
@hoxbro hoxbro deleted the cleanup_popup branch May 7, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants