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

Improve the FileDownload widget #1306

Merged
merged 4 commits into from
Apr 28, 2020

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Apr 28, 2020

Fixes #1299

The intent of this PR is to fix various bugs with the FileDownload widget. I had to clearly define the behaviour of the widget, that to me follows three cases:

  1. embed=True --> The widget is a link that looks like a button (right click to "Save as..." is possible)
  2. auto=False --> There are two steps, first the widget is a button, after it's clicked it's a link (right click possible)
  3. auto=True --> The widget is a button (right click to "Save as..." isn't possible).

To get there I had to make the following changes:

  • On the python side: the biggest change is the new _transfers model attribute. It took me a long time before I dared to touch the python side but I found out it was the only solution that worked and allowed to fix all the bugs. When the widget is a button, a click will increment _clicks in typescript which will be reflected in python and trigger _transfer(). This method will increment _transfers, this signal will be caught on the typescript side, where a link will be created and clicked to download the data.
  • On the typescript side: I've opted for an approach that is in favour of rendering first the widget and then updating it (that is not completely true as disabling/enabling the widget actually renders it again because of an inherited callback). It's quite a change compared to the first implementation, I believe it was required to cover all the cases supposedly offered by the widget.

Example fixed:
filedownload-fixed-example

Updating button type, disabled, filename fixed:
filedownload-fixed-ui

@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #1306 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1306      +/-   ##
==========================================
+ Coverage   87.77%   87.81%   +0.04%     
==========================================
  Files         124      124              
  Lines       13485    13509      +24     
==========================================
+ Hits        11836    11863      +27     
+ Misses       1649     1646       -3     
Impacted Files Coverage Δ
panel/models/widgets.py 100.00% <100.00%> (ø)
panel/tests/widgets/test_misc.py 98.11% <100.00%> (+0.38%) ⬆️
panel/widgets/misc.py 93.08% <100.00%> (+2.17%) ⬆️

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 5282159...6962b6e. Read the comment docs.

@philippjfr
Copy link
Member

Thanks for this, not an easy fix to make after all. The approach makes a lot of sense to me though so just let me know if you think it's ready.

@maximlt
Copy link
Member Author

maximlt commented Apr 28, 2020

Relieved that you're OK with all those changes!

I've seen some typos and things to be improved in the docs (the second sentence isn't complete here). I'll fix that and then it's ready.

@maximlt
Copy link
Member Author

maximlt commented Apr 28, 2020

I think it's ready now.

@philippjfr
Copy link
Member

Looks ready to me, thanks so much for contributing!

@philippjfr philippjfr merged commit 0378ae6 into holoviz:master Apr 28, 2020
@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented Apr 29, 2020

So great to see this merged. Just had feedback from one of my users that experienced this bug.

philippjfr pushed a commit that referenced this pull request May 24, 2020
* Fix filename updating

* Monitor when _transfers() runs

* Fix various issues with the filedownload widget

* Fix the FileDownload docs
philippjfr pushed a commit that referenced this pull request Jun 19, 2020
* Fix filename updating

* Monitor when _transfers() runs

* Fix various issues with the filedownload widget

* Fix the FileDownload docs
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.

List of bugs related to the FileDownload widget
3 participants