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

[Zeppelin-2572] multiple paragraphs actions #2952

Closed
wants to merge 12 commits into from

Conversation

Savalek
Copy link
Contributor

@Savalek Savalek commented Apr 28, 2018

What is this PR for?

This PR adds the ability to perform actions on several paragraphs simultaneously. He adds the checkbox to each paragraph. When you select at least one paragraph in the action bar, buttons appear that perform actions on the selected paragraphs.

What type of PR is it?

Feature

What is the Jira issue?

ZEPPELIN-2572

Screenshots (if appropriate)

33115591-993e6cf4-cf72-11e7-8987-64cb3e137ba8

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

# Conflicts:
#	zeppelin-integration/src/test/java/org/apache/zeppelin/integration/ParagraphActionsIT.java
#	zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/socket/Message.java
@r-kamath
Copy link
Member

r-kamath commented May 2, 2018

Run all/selected button behavior is not right when selected paragraphs are removed. Until reset selections/refresh the page retains the selection count.
runall

@Savalek
Copy link
Contributor Author

Savalek commented May 3, 2018

@r-kamath, I fixed this flaw.

@@ -291,6 +291,9 @@ public void onMessage(NotebookSocket conn, String msg) {
case MOVE_PARAGRAPH:
moveParagraph(conn, userAndRoles, notebook, messagereceived);
break;
case MOVE_SEVERAL_PARAGRAPHS:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use MOVE_PARAGRAPHS for moving multiple paragraphs?

@@ -1641,6 +1686,35 @@ private void moveParagraph(NotebookSocket conn, HashSet<String> userAndRoles, No
new Message(OP.PARAGRAPH_MOVED).put("id", paragraphId).put("index", newIndex));
}

private void moveSeveralParagraphs(NotebookSocket conn, HashSet<String> userAndRoles,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moveParagraphs

ng-hide="viewOnly"
tooltip-placement="bottom" uib-tooltip="Enable/Disable for running"
ng-disabled="revisionView">
<i ng-class="isAnableRun ? 'fa fa-lock' : 'fa fa-unlock'"></i>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isEnableRun

@Savalek
Copy link
Contributor Author

Savalek commented May 7, 2018

@r-kamath, I fixed it.

@r-kamath
Copy link
Member

r-kamath commented May 8, 2018

LGTM

@r-kamath
Copy link
Member

r-kamath commented May 9, 2018

@jongyoul @zjffdu @prabhjyotsingh please review

Copy link
Contributor

@Tagar Tagar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. left one minor spelling comment.
tested this manually. will deploy to users soon.
great improvement.

data-toggle="modal" data-target="#noteCreateModal" data-clone-note="true"
data-only-selected-paragraphs="true"
data-selected-paragraphs="{{getSelectedParagraphs()}}"
tooltip-placement="bottom" uib-tooltip="Clone this note use selected paragraphs">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: Clone this note using selected paragraphs

ng-class="{'disabled':isNoteRunning()}"
tooltip-placement="bottom" uib-tooltip="Run all paragraphs"
tooltip-placement="bottom"
uib-tooltip="{{isSelectionMode() ? 'Run selected paragraph' : 'Run all paragraphs'}}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

selected paragraphs

class="btn btn-default btn-xs"
ng-if="isSelectionMode()"
ng-click="removeSelectedParagraph(note.id)"
tooltip-placement="bottom" uib-tooltip="Delete selected paragraph">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"delete selected paragraphs"

@Tagar
Copy link
Contributor

Tagar commented May 25, 2018

thank you @Savalek

@zjffdu would it be possible to merge this to master? (and not to 0.8)
thanks!

# Conflicts:
#	zeppelin-web/src/app/notebook/notebook.controller.js
#	zeppelin-web/src/app/notebook/notebook.css
<span class="{{selectedParagraphsIds.has(paragraph.id) ? 'icon-check checked-paragraph-check-button' : 'icon-plus'}}"
tooltip-placement="top"
style="cursor:pointer"
uib-tooltip="Select a paragraph"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Add this paragraph to selection"

@Tagar
Copy link
Contributor

Tagar commented Jun 5, 2018

We were using PR this in prod for a couple of weeks. Works great.
Left one additional comment based on users feedback.
"Select a paragraph" tooltip change to "Add this paragraph to selection" would be better,
as it selects that particular paragraph and based on tooltip users can see that it talks
about multi-paragraph selection.

@Savalek
Copy link
Contributor Author

Savalek commented Jun 6, 2018

@Tagar, thanks. I changed the tooltip.

# Conflicts:
#	zeppelin-server/src/main/java/org/apache/zeppelin/socket/NotebookServer.java
#	zeppelin-web/src/components/websocket/websocket-event.factory.js
#	zeppelin-zengine/src/main/java/org/apache/zeppelin/notebook/socket/Message.java
@jongyoul
Copy link
Member

jongyoul commented Jul 6, 2018

Let me test it in my local

@Savalek
Copy link
Contributor Author

Savalek commented Aug 7, 2018

Changed the UI and add shortcut to select/deselect paragraph.
multi

@jongyoul, what you say about it?

@@ -191,7 +191,13 @@
INTERPRETER_INSTALL_RESULT, // [s-c] Status of an interpreter installation
COLLABORATIVE_MODE_STATUS, // [s-c] collaborative mode status
PATCH_PARAGRAPH, // [c-s][s-c] patch editor text
NOTICE // [s-c] Notice
NOTICE, // [s-c] Notice
REMOVE_SELECTED_PARAGRAPHS, // [c-s] remove selected paragraphs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge these with the single paragraph operation to reduce code duplication ? Otherwise if we introduce new operation in future, we have to introduce 2 message, one for single paragraph, another for multiple paragraph, which is inefficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'll think about how to improve this.

@Savalek Savalek closed this Aug 16, 2018
@Savalek
Copy link
Contributor Author

Savalek commented Aug 16, 2018

refactored and transferred to Zeppelin-2572 ver2

Savalek added a commit to Tinkoff/zeppelin that referenced this pull request Nov 7, 2018
### What is this PR for?
This PR adds the ability to perform actions on several paragraphs simultaneously. He adds the checkbox to each paragraph. When you select at least one paragraph in the action bar, buttons appear that perform actions on the selected paragraphs.

### What type of PR is it?
Feature

### What is the Jira issue?
[ZEPPELIN-2572](https://issues.apache.org/jira/browse/ZEPPELIN-2572)
ZP-24
apache#2952 declined.
apache#3145
#6

### Screenshots (if appropriate)
![mult_v2](https://user-images.githubusercontent.com/30798933/44201040-e4a0d000-a150-11e8-955c-2837683f4651.gif)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no
Savalek added a commit to Tinkoff/zeppelin that referenced this pull request Nov 7, 2018
### What is this PR for?
This PR adds the ability to perform actions on several paragraphs simultaneously. He adds the checkbox to each paragraph. When you select at least one paragraph in the action bar, buttons appear that perform actions on the selected paragraphs.

### What type of PR is it?
Feature

### What is the Jira issue?
[ZEPPELIN-2572](https://issues.apache.org/jira/browse/ZEPPELIN-2572)
ZP-24
apache#2952 declined.
apache#3145
#6

### Screenshots (if appropriate)
![mult_v2](https://user-images.githubusercontent.com/30798933/44201040-e4a0d000-a150-11e8-955c-2837683f4651.gif)

### Questions:
* Does the licenses files need update? no
* Is there breaking changes for older versions? no
* Does this needs documentation? no
@Savalek Savalek deleted the ZEPPELIN-2572 branch January 25, 2019 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants