You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zeppelin.apache.org by sanjaydasgupta <gi...@git.apache.org> on 2018/06/10 05:55:05 UTC
[GitHub] zeppelin pull request #3013: [ZEPPELIN-3511] remove old button "Download Dat...
GitHub user sanjaydasgupta opened a pull request:
https://github.com/apache/zeppelin/pull/3013
[ZEPPELIN-3511] remove old button "Download Data as CSV/TSV"
### What is this PR for?
This PR removes the legacy method (button and dropdown near the top left of each paragraph) for exporting the displayed data as a CSV or TSV file. The widgets of the legacy method are seen in the images below.
The legacy method is now redundant as a new improved data-export menu is available (top right corner of data display grid). The new method also does not suffer from certain issues in the legacy method. The new method is seen in the image below.
### What type of PR is it?
[Improvement]
### Todos
### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-3511
### How should this be tested?
1) Travis CI pass
2) Manual checking (see screenshot below)
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sanjaydasgupta/zeppelin z3511-remove-old-download-button
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zeppelin/pull/3013.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3013
----
commit cabb83e82a2e77326e214a6e506e53f6fb00ff07
Author: Sanjay Dasgupta <sa...@...>
Date: 2018-06-10T05:32:22Z
z3511-remove-old-download-button: Initial updates
----
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by Tagar <gi...@git.apache.org>.
Github user Tagar commented on the issue:
https://github.com/apache/zeppelin/pull/3013
my 2 cents: 1. yes. 2,3. yes. except that `grid-ui` doesn't have native `tsv` export, right? I thought `grid-ui` only allows to export as `csv` and as `xlsx`, is this correct? If it is, we might just drop `tsv` export. I personally never used it. `csv` and `xlsx` exports through `grid-ui` should be enough.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
I agree that the hook up `ui-grid` from `old button` makes sense. @Tagar @sanjaydasgupta
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
@sanjaydasgupta As a longterm plan, I like to remove old button as it's not maintained at all for now. But discussing those kinds of issues in a PR is not proper. How about moving this discussion on dev@? We would remove old download button if we could make a consensus on the mailing list.
BTW, personally, I give my 2 cents to remove old button. :-)
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by Tagar <gi...@git.apache.org>.
Github user Tagar commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Was thinking of another solution.. is it possible to *move* grid-ui's
- Export all/visible data as csv/xls
four options to that old Download menu?
So grid-ui would only have more rare-used "Columns: .. " items, and
export items will be moves completely to that older button that users got used to.
So old button would exposes newer angular-ui-grid export functionality.
Thoughts?
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
@jongyoul, @Tagar, could you please confirm my understanding of your suggestions below:
1) The old button should **not** be removed. It should retain its visual form and function (although the implementation of its functions may change)
2) The CSV and TSV saving functions of the old button should reuse and invoke corresponding functions of the angular ui-grid.
3) To implement the TSV part of (2) two extra items (_Export all data as tsv_, and _Export visible data as tsv_) should be added to ui-grid's menu item.
If your answer to most of the above points is _yes_, we should close this PR as it has traveled a long way down the wrong path :-) But before we do that let me request you to review once more the motivations in the JIRA ([ZEPPELIN-3511](https://issues.apache.org/jira/browse/ZEPPELIN-3511)) and the long term maintainability benefits of removing the old button (also emphasized [here](https://github.com/apache/zeppelin/pull/3013#issuecomment-397788053)).
Thank you for your attention.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/3013
any update?
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
I remembered it didn't have any objection for it.
On Tue, Oct 16, 2018 at 3:57 PM Felix Cheung <no...@github.com>
wrote:
> possibly? any objection from reviewers of this change/PR?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/zeppelin/pull/3013#issuecomment-430122759>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ADcflggQ7_wES-1L6ZyPgg1NjJjvrPLnks5ulYNFgaJpZM4UhmEQ>
> .
>
--
이종열, Jongyoul Lee, 李宗烈
http://madeng.net
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Tested this PR locally on FF, Safari, Chrome, and Edge25 works well.
The only concern over here is which one should we be removing the "legacy method" or the one that is avail with the data-grid table. I mean the "legacy method" is always shown even when the visualization is selected, and the download button on data-grid get hidden as soon as visualization.
If we all agree to this then LGTM.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/3013
possibly? any objection from reviewers of this change/PR?
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/zeppelin/pull/3013
ok, let's go ahead then?
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
As suggested by @jongyoul earlier, a mail was sent out to the dev list on 9th July 2018 with this subject: Should we remove "Download Data as CSV/TSV" button from version 0.8.0?
But there was very little interest from the community, with just 3 responses (including my own) -- all favoring removal of the old button.
I was not sure if 3 positive votes (and silence from a couple of others who had dissented earlier) is a strong enough indicator in favor of the motion.
Any additional guidance will help :-)
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
@jongyoul I agree.
Will return to this in a day or two.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Please see my suggestion (described above) implemented in the image below. The legacy method's button (with the very familiar download icon) has been retained with a different tool-tip message, and clicking on the button has a different effect - switches to the grid view. If this button is retained for 1 or 2 releases, it will help existing users to discover and transition to the new ui-grid menu.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
@sanjaydasgupta I'm not sure your option is better than noticing with a release message. Generally, we add breaking change message like this kind of modification. Personally, I like to remove this button and use grid's download, mainly because of 2.c.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by Tagar <gi...@git.apache.org>.
Github user Tagar commented on the issue:
https://github.com/apache/zeppelin/pull/3013
@sanjaydasgupta I agree with what you're saying
but what I was suggesting is just hook up **old button** to **ui-grid**
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Thanks @prabhjyotsingh.
I will wait for a clear common view before making any further changes.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
I have been a little busy @jongyoul, but have not forgotten it ;-)
Will be done today. Thanks.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Sure ~
On Wed, Oct 17, 2018 at 3:22 PM Felix Cheung <no...@github.com>
wrote:
> ok, let's go ahead then?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/zeppelin/pull/3013#issuecomment-430504087>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/ADcflkZ9hbPjuVQBEul6pFHlLhF8bJueks5ulsycgaJpZM4UhmEQ>
> .
>
--
이종열, Jongyoul Lee, 李宗烈
http://madeng.net
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Allow me to remind you of the history of this PR:
1) There was an [initial commit](https://github.com/apache/zeppelin/pull/3013/commits/cabb83e82a2e77326e214a6e506e53f6fb00ff07) which just removed the old button and did nothing else (perhaps as intended by the author of [the jira](https://issues.apache.org/jira/browse/ZEPPELIN-3511))
2) I also made another commit providing a [transition proposal](https://github.com/apache/zeppelin/pull/3013/commits/bee502e0148b2c8222be6c23f157aaf45e3fb418) to guide existing users who may miss the old button
There has been a suggestion from @jongyoul that the item at (2) above is not desirable, so should I remove that piece?
Thanks for your guidance.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by prabhjyotsingh <gi...@git.apache.org>.
Github user prabhjyotsingh commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Sure, looks good.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
LGTM, @prabhjyotsingh Could you please verify it finally? I'll merge it there's no more discussion.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
@sanjaydasgupta Hi, I missed this issue fully. So sorry. I think there was no objection about this issue. Correct?
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Thanks for the input @prabhjyotsingh.
Allow me to list the advantages of both methods, and to suggest a possible way forward for your consideration.
1) **The legacy method has two big advantages**:
a) It is well known, so making it disappear without notice will be a very unpleasant jolt to all existing users.
b) Being always available, it is usable when either the grid or any visualization is displayed.
2) **The ui-grid menu approach has the following advantages**:
a) It does not suffer from the problems mentioned in the [issue](https://issues.apache.org/jira/browse/ZEPPELIN-3511) - though one of them has been fixed.
b) It allows downloading to an Excel file as well.
c) Being based on a 3rd party package, the support effort will be less, and periodic enhancements can be expected automatically.
Considering everything, it would seem that the advantage in 1(a) will be the main loss. The loss of 1(b) will become acceptable once users are familiar with the new method, as users can switch to the grid to download data.
If we agree to move progressively towards the ui-grid menu approach (without confusing users who are familiar with the legacy method) the following could be an option:
In place of the legacy method button and dropdown we add a label with a tool-tip popup that informs the user where the data-download method has moved. This informative label can be maintained for one or two releases, by when all users will be familiar with the new menu. The label can then be removed.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by sanjaydasgupta <gi...@git.apache.org>.
Github user sanjaydasgupta commented on the issue:
https://github.com/apache/zeppelin/pull/3013
Thanks for the ideas @Tagar
If we focus on long term advantages and stability, then continuing to maintain the old button will be a disadvantage. If the old button is retained, it will have to be revisited (implement, test, document) every time the [ui-grid](http://ui-grid.info/) menu adds a new feature or changes an existing feature. This is clearly not desirable, and should not be our choice of the way forward.
I had offered to provide a transition mechanism based on a visually similar button, but just as a transition mechanism - to be removed in 1 or 2 releases.
The ui-grid already has some nice features (like PDF output) that zeppelin users have requested, and going with ui-grid will IMHO be a good decision.
---
[GitHub] zeppelin issue #3013: [ZEPPELIN-3511] remove old button "Download Data as CS...
Posted by jongyoul <gi...@git.apache.org>.
Github user jongyoul commented on the issue:
https://github.com/apache/zeppelin/pull/3013
ping @sanjaydasgupta
---