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

Fix handling of the Toggle widget in a Param pane #1342

Merged
merged 2 commits into from
May 13, 2020

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented May 13, 2020

Fixes #1331

The panel widget Toggle was not synced like the other widgets in a Param pane:

panel/panel/param.py

Lines 378 to 379 in b73ec77

if isinstance(widget, Toggle):
pass

This special handling originated from #15 and was probably implemented this way because the widget that is created when one needs to expand a subparameter pane is a Toggle. I believe that now widgets created in a pane are given by _ordered_params() which do not expose that Toggle widget. So it's just fine to remove that special handling.

I also took advantage of this PR to fix a typo and improve the coverage of the param action test.

@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #1342 into master will increase coverage by 0.01%.
The diff coverage is 98.64%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1342      +/-   ##
==========================================
+ Coverage   87.49%   87.51%   +0.01%     
==========================================
  Files         130      130              
  Lines       13758    13759       +1     
==========================================
+ Hits        12038    12041       +3     
+ Misses       1720     1718       -2     
Impacted Files Coverage Δ
panel/param.py 91.85% <98.50%> (+0.41%) ⬆️
panel/tests/test_param.py 99.85% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b73ec77...51bb735. Read the comment docs.

@philippjfr
Copy link
Member

Thanks!

This special handling originated from #15 and was probably implemented this way because the widget that is created when one needs to expand a subparameter pane is a Toggle.

That is correct.

I believe that now widgets created in a pane are given by _ordered_params() which do not expose that Toggle widget. So it's just fine to remove that special handling.

That sounds right, but just to be sure do you know if this is currently under test and/or have you tested subobject toggling manually?

@maximlt
Copy link
Member Author

maximlt commented May 13, 2020

Hi @philippjfr and thanks for your feedback,

I've tested it manually and saw nothing wrong.

Some of the current tests emulate a click on the toggle button, see for example:

test_pane._widgets['a'][1].value = True

I didn't have to change them so I assumed everything was alright.

If you'd like me to add another test that's OK. I'd just need some guidance as I don't really know how to check that. Could it be as fine grained as checking that there is just one callback on the toggle button?

@philippjfr
Copy link
Member

I'm pretty sure it's fine too after looking at it so I'll merge. Thanks again!

@philippjfr philippjfr merged commit 489cc0c into holoviz:master May 13, 2020
philippjfr pushed a commit that referenced this pull request May 24, 2020
* Improve action param test

* Fix toggle widget handling in Param
philippjfr pushed a commit that referenced this pull request Jun 19, 2020
* Improve action param test

* Fix toggle widget handling in Param
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.

pn.param mapping of boolean to toggle fails with param.depends
2 participants