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.

Sign up for GitHub

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

Jump to bottom

[ZEPPELIN-1309] Show checkbox for "Connect to existing process" on interpreter menu. #1229

Closed
wants to merge 4 commits into from
Closed

[ZEPPELIN-1309] Show checkbox for "Connect to existing process" on interpreter menu. #1229

wants to merge 4 commits into from

Conversation

astroshim
Copy link
Contributor

@astroshim astroshim commented Jul 26, 2016 •
edited

What is this PR for?

This PR fixes a bug of showing checkbox for "Connect to existing process" on interpreter menu.

What type of PR is it?

Bug Fix

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-1309

How should this be tested?

Go to interpreter menu and check if checkbox for "Connect to existing process" exists.

Screenshots (if appropriate)

  • before
    image
  • after
    image

Questions:

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

@bzz
Copy link
Member

bzz commented Jul 28, 2016

Looks good to me.
\cc @corneadoug to double-check

Merging if there is no further discussion

corneadoug
corneadoug reviewed Jul 28, 2016
View reviewed changes
zeppelin-web/src/app/interpreter/interpreter.html
<span class="input-group">
<label><input type="checkbox" style="width:0%;height:0%" id="isExistingProcess" ng-model="setting.option.isExistingProcess" ng-disabled="!valueform.$visible"/>
<span class="input-group" style="line-height:30px;">
<label><input type="checkbox" style="width:10%;height:70%" id="isExistingProcess" ng-model="setting.option.isExistingProcess" ng-disabled="!valueform.$visible"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole style (width and height), can actually be removed from here

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 for giving your advice.
You mean the style="line-height:30px;" can be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

No this one is needed.
On the line under style="width:10%;height:70%"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay i see. I think height can be removed but width can not be removed.
because if remove width and height both, the screen is following like.
image
so I think width is needed. What do you think?

@astroshim
Copy link
Contributor Author

re-trigger CI

@astroshim astroshim closed this Aug 1, 2016
@astroshim astroshim reopened this Aug 1, 2016
@astroshim
Copy link
Contributor Author

re-trigger CI

@astroshim astroshim closed this Aug 1, 2016
@astroshim astroshim reopened this Aug 1, 2016
corneadoug
corneadoug reviewed Aug 2, 2016
View reviewed changes
zeppelin-web/src/app/interpreter/interpreter.html
<span class="input-group">
<label><input type="checkbox" style="width:0%;height:0%" id="isExistingProcess" ng-model="setting.option.isExistingProcess" ng-disabled="!valueform.$visible"/>
<span class="input-group" style="line-height:30px;">
<label><input type="checkbox" style="width:10%" id="isExistingProcess" ng-model="setting.option.isExistingProcess" ng-disabled="!valueform.$visible"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why leaving width here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because without width, it will be as following image.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be a Firefox only bug. Let's replace 10% by 20px then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed. Thank you!

@corneadoug
Copy link
Contributor

LGTM

@corneadoug
Copy link
Contributor

Merging if there is no more discussions

@asfgit asfgit closed this in 55695c9 Aug 7, 2016
asfgit pushed a commit that referenced this pull request Aug 7, 2016
@corneadoug
[BugFix] Show checkbox for "Connect to existing process" on interpret…
115fb24 
…er menu.

This PR fixes a bug of showing checkbox for "Connect to existing process" on interpreter menu.

Bug Fix

Go to interpreter menu and check if checkbox for "Connect to existing process" exists.

- before
![image](https://cloud.githubusercontent.com/assets/3348133/17141824/aa316672-5388-11e6-952d-50a3769a01a5.png)

- after
![image](https://cloud.githubusercontent.com/assets/3348133/17141840/babfad8c-5388-11e6-9209-e37e252e95b4.png)

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

Author: astroshim <hsshim@nflabs.com>

Closes #1229 from astroshim/bugfix/showCheckbox and squashes the following commits:

4b3673a [astroshim] add css to interpreter-create page
17fb0d7 [astroshim] change size to px
47ce719 [astroshim] remove height style on the checkbox
bbc3283 [astroshim] show checkbox on interpreter menu.

(cherry picked from commit 55695c9)
Signed-off-by: Damien CORNEAU <corneadoug@gmail.com>
@corneadoug
Copy link
Contributor

Merged in Master and branch-0.6

@corneadoug
Copy link
Contributor

@astroshim Was there a JIRA issue?

PhilippGrulich pushed a commit to SWC-SENSE/zeppelin that referenced this pull request Aug 8, 2016
@PhilippGrulich
[BugFix] Show checkbox for "Connect to existing process" on interpret…
75efbeb 
…er menu.

### What is this PR for?
This PR fixes a bug of showing checkbox for "Connect to existing process" on interpreter menu.

### What type of PR is it?
Bug Fix

### How should this be tested?
Go to interpreter menu and check if checkbox for "Connect to existing process" exists.

### Screenshots (if appropriate)
- before
![image](https://cloud.githubusercontent.com/assets/3348133/17141824/aa316672-5388-11e6-952d-50a3769a01a5.png)

- after
![image](https://cloud.githubusercontent.com/assets/3348133/17141840/babfad8c-5388-11e6-9209-e37e252e95b4.png)

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

Author: astroshim <hsshim@nflabs.com>

Closes apache#1229 from astroshim/bugfix/showCheckbox and squashes the following commits:

4b3673a [astroshim] add css to interpreter-create page
17fb0d7 [astroshim] change size to px
47ce719 [astroshim] remove height style on the checkbox
bbc3283 [astroshim] show checkbox on interpreter menu.
@corneadoug
Copy link
Contributor

@astroshim Can you create a JIRA issue for this, or point me to the existing one?

@astroshim
Copy link
Contributor Author

okay I'll make it.

@astroshim astroshim changed the title [BugFix] Show checkbox for "Connect to existing process" on interpreter menu. [ZEPPELIN-1309] Show checkbox for "Connect to existing process" on interpreter menu. Aug 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@astroshim @bzz @corneadoug