Skip to content

ZEPPELIN-508 Interpreter binding cancel button doesn't work#548

Closed
AhyoungRyu wants to merge 6 commits intoapache:masterfrom
AhyoungRyu:ZEPPELIN-508
Closed

ZEPPELIN-508 Interpreter binding cancel button doesn't work#548
AhyoungRyu wants to merge 6 commits intoapache:masterfrom
AhyoungRyu:ZEPPELIN-508

Conversation

@AhyoungRyu
Copy link
Contributor

What is this PR for?

Currently, when there is no change compared with before the interpreter binding setting status, the cancel button in binding page doesn't work. So I fixed this bug.

What type of PR is it?

Bug Fix

Todos

  • - Fix the cancel button in interpreter binding page.

Is there a relevant Jira issue?

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

How should this be tested?

Open the Zeppelin any notebook page -> Click the interpreter binding button

  1. Don't change any binding setting and Just click the cancel button.
  2. Or just click twice the interpreter binding button.

Screenshots (if appropriate)

After this PR applied,
zeppelin508_after 1

Questions:

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

Copy link
Member

Choose a reason for hiding this comment

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

Just $scope.showSetting = false; wouldn't be enough?
Personally I wouldn't expect alert message in this case.
And after clicking cancel, having 'cancel' and 'ok' button choice feels a bit awkward for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Mina, no need for Dialog here

@AhyoungRyu
Copy link
Contributor Author

Thanks for the review @minahlee @corneadoug.
I applied the reviews. : )

@Leemoonsoo
Copy link
Member

Thanks for the fix.
LGTM. I'm merging it into master.

@asfgit asfgit closed this in 0194d38 Dec 21, 2015
@AhyoungRyu AhyoungRyu deleted the ZEPPELIN-508 branch December 21, 2015 07:29
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.

4 participants