Skip to content

Conversation

@khalidhuseynov
Copy link
Member

@khalidhuseynov khalidhuseynov commented Jul 11, 2016

What is this PR for?

This PR addresses ability to view the history of note revisions from the version control menu

What type of PR is it?

Improvement

Todos

  • - backend changes
  • - tests
  • - propagation to frontend
  • - rendering list

What is the Jira issue?

Zeppelin - 1152

How should this be tested?

  1. Select Git notebook storage layer as primary (e.g. in conf/zeppelin-env.sh add
export ZEPPELIN_NOTEBOOK_STORAGE="org.apache.zeppelin.notebook.repo.GitNotebookRepo"
  1. Select any note in Zeppelin and commit it in version control menu
  2. The list with new commit should be shown in the same menu

Screenshots (if appropriate)

screen shot 2016-07-21 at 11 35 21 am

### Questions: - Does the licenses files need update? No - Is there breaking changes for older versions? No - Does this needs documentation? No

@corneadoug
Copy link
Contributor

screen shot 2016-07-12 at 5 42 56 pm

I made a dropdown for the revisions, so that it always show current one.
The caret button is only shown if there is some revisions.

Current dropdown format is: Date - Commit message

@corneadoug
Copy link
Contributor

@khalidhuseynov All builds are red, can you check it out?

@khalidhuseynov khalidhuseynov changed the title WIP [Zeppelin - 1152] Listing note revision history [Zeppelin - 1152] Listing note revision history Jul 14, 2016
@khalidhuseynov
Copy link
Member Author

@corneadoug thanks for the help with rendering list, and i've fixed the tests. CI is green and ready for review.

@Leemoonsoo
Copy link
Member

Looks good. Can we display time as well as date in each revision?

@khalidhuseynov khalidhuseynov force-pushed the front/list-revision-history branch from 5049163 to c437017 Compare July 18, 2016 17:07
@khalidhuseynov khalidhuseynov force-pushed the front/list-revision-history branch from 88faf78 to 4607c03 Compare July 19, 2016 07:41
@khalidhuseynov
Copy link
Member Author

@Leemoonsoo updated code and screenshot with feedback

@khalidhuseynov khalidhuseynov force-pushed the front/list-revision-history branch from 00a5dc5 to f676b74 Compare July 20, 2016 04:19
@corneadoug
Copy link
Contributor

Is it better to have the Date potentially hidden or the title hidden?

@anthonycorbacho
Copy link
Contributor

anthonycorbacho commented Jul 20, 2016

@khalidhuseynov what is the benefit of having a new websocket call? Why not use rest-api?

@khalidhuseynov
Copy link
Member Author

@corneadoug i'm not sure exactly what you mean, could you provide an example?

@anthonycorbacho because:

  • there're cases when backend will send the list triggered by different ws event (checkpoint) for example here
  • to have consistency with original work and provide whole api through websocket, possibly later provide rest-api as well

@corneadoug
Copy link
Contributor

@khalidhuseynov Previous order in the list was: Date - Title, now it is Title - Date.
As you can see in your screenshot, when the text gets too long, there is an ellipse:

add visualiz paragraph - July 19th 2016, 5...

@khalidhuseynov
Copy link
Member Author

@corneadoug yeah i changed the order since it's more intuitive to have name first, and date afterwards. But you're right when too long name, the ellipse doesn't look good. how do you think we can fix it; maybe move date to new line, or any other suggestions?

@Leemoonsoo
Copy link
Member

@corneadoug @khalidhuseynov

Or how about align commit message left, and align date right and use fixed length date format?
Then it gives little better readability i think.

add visualize paragrgraph...    2016-07-06 10:12:34
add tutorial                    2016-07-06 09:11:11
the first commit                2016-05-23 23:31:33

@khalidhuseynov
Copy link
Member Author

okay, i'll play with it a bit then

@khalidhuseynov
Copy link
Member Author

@corneadoug @Leemoonsoo @anthonycorbacho how about this one?
screen shot 2016-07-20 at 4 40 09 pm

@corneadoug
Copy link
Contributor

@khalidhuseynov looks good, but i would reduce the font size and spacing

@khalidhuseynov
Copy link
Member Author

@corneadoug how about this one 614ed61 :
screen shot 2016-07-20 at 5 29 09 pm

@bzz
Copy link
Member

bzz commented Jul 20, 2016

Great improvement! In this case WS protocol improvement makes sense indeed.

👍 for tests

CI is failing on

Results :

Failed tests: 
  ZeppelinSparkClusterTest.sparkRTest:104 expected:<FINISHED> but was:<ERROR>

Tests run: 65, Failures: 1, Errors: 0, Skipped: 0

which looks like a flaky test to me.

@khalidhuseynov khalidhuseynov force-pushed the front/list-revision-history branch from 01343ca to 924a6c5 Compare July 21, 2016 04:55
@khalidhuseynov
Copy link
Member Author

khalidhuseynov commented Jul 21, 2016

addressed comments and CI is green. updated description screenshot as well, /cc @corneadoug @Leemoonsoo @bzz for review

@corneadoug
Copy link
Contributor

LGTM

@bzz
Copy link
Member

bzz commented Jul 21, 2016

Looks good to me, merging if there is no further discussion

@asfgit asfgit closed this in 5cdc02d Jul 21, 2016
@khalidhuseynov khalidhuseynov deleted the front/list-revision-history branch July 21, 2016 15:55
PhilippGrulich pushed a commit to SWC-SENSE/zeppelin that referenced this pull request Aug 8, 2016
### What is this PR for?
This PR addresses ability to view the history of note revisions from the `version control` menu

### What type of PR is it?
Improvement

### Todos
* [x] - backend changes
* [x] - tests
* [x] - propagation to frontend
* [x] -  rendering list

### What is the Jira issue?
[Zeppelin - 1152](https://issues.apache.org/jira/browse/ZEPPELIN-1152)

### How should this be tested?
1. Select Git notebook storage layer as primary (e.g. in `conf/zeppelin-env.sh` add
```
export ZEPPELIN_NOTEBOOK_STORAGE="org.apache.zeppelin.notebook.repo.GitNotebookRepo"
```
2. Select any note in Zeppelin and `commit` it in `version control` menu
3. The list with new commit should be shown in the same menu

### Screenshots (if appropriate)
<img width="782" alt="screen shot 2016-07-21 at 11 35 21 am" src="https://cloud.githubusercontent.com/assets/1642088/17009639/c8bcbd58-4f37-11e6-92cd-88147b7419e6.png">

### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Author: Khalid Huseynov <[email protected]>
Author: Khalid Huseynov <[email protected]>
Author: Damien CORNEAU <[email protected]>

Closes apache#1160 from khalidhuseynov/front/list-revision-history and squashes the following commits:

924a6c5 [Khalid Huseynov] address @corneadoug comments
614ed61 [Khalid Huseynov] date: smaller font and spacing
f676b74 [Khalid Huseynov] Merge branch 'master' into front/list-revision-history
bd068fd [Khalid Huseynov] add css class with smaller font for revision date
0445b42 [Khalid Huseynov] move date to right, add time
4607c03 [Khalid Huseynov] fix css
e6bafde [Khalid Huseynov] change time format
2090d60 [Khalid Huseynov] mock listRevisionHistory
1d20aeb [Khalid Huseynov] fix jshint checkstyle
ee7b94f [Damien CORNEAU] Reverse revisions order to have the latest first
9523b48 [Damien CORNEAU] Add dropdown of revisions
660139b [Damien CORNEAU] fix the jscs error
bcf6cf1 [Khalid Huseynov] return list on successful commit
18d430a [Khalid Huseynov] add initial listing
b4c69df [Khalid Huseynov] add test: revision history, multiple notes
d8f4282 [Khalid Huseynov] add new note resource for tests with multiple notes
a7e8fd8 [Khalid Huseynov] initial impl
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.

5 participants