Skip to content

[ZEPPELIN-696] Add notification system for AngularJS z functions - #744

Closed
doanduyhai wants to merge 1 commit into
apache:masterfrom
doanduyhai:ZEPPELIN-696
Closed

[ZEPPELIN-696] Add notification system for AngularJS z functions#744
doanduyhai wants to merge 1 commit into
apache:masterfrom
doanduyhai:ZEPPELIN-696

Conversation

@doanduyhai

Copy link
Copy Markdown
Contributor

What is this PR for?

Add notification system for AngularJS z functions. Now that we expose the z Angular object, we should also catch error/exceptions and display error messages properly to the user.

I changed a little bit the default settings for ngToast. Instead of having notifications on bottom right, I let the default value undefined.

For Interpreter page notification, I force the position to bottom right as before when using default config.

For z functions error messages, I choose to display notifications on top right corner because it's clearer. See screen capture

This is a sub-task of epic ZEPPELIN-635

What type of PR is it?

[Improvement]

Todos

  • - Code Review
  • - Simple Test

Is there a relevant Jira issue?

ZEPPELIN-696

How should this be tested?

  • git fetch origin pull/744/head:AngularJSNotification
  • git checkout AngularJSNotification
  • mvn clean package -DskipTests
  • bin/zeppelin-daemon.sh restart
  • Create a new note
  • In the first paragraph, put the following code
%angular
<formclass="form-inline"><buttontype="submit" class="btn btn-primary" ng-click="z.runParagraph()"> Run Paragraph Without Id</button><buttontype="submit" class="btn btn-primary" ng-click="z.runParagraph('20160223-141919_305734436')"> Run Paragraph Without Wrong Id</button><buttontype="submit" class="btn btn-primary" ng-click="z.angularBind('superhero', superhero)"> Angular Bind Without Paragraph Id</button><buttontype="submit" class="btn btn-primary" ng-click="z.angularUnbind('superhero')"> Angular UnBind Without Paragraph Id</button></form>
  • Click on each button to see the appropriate error message

Screenshots (if appropriate)

angularnotification

Questions:

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

@corneadoug

Copy link
Copy Markdown
Contributor

We already have ngtoast for that

@doanduyhai

Copy link
Copy Markdown
ContributorAuthor

Right, didn't know that we had ngToast. I never see it in action. Will remove angular-growl-2

@doanduyhai
doanduyhaiforce-pushed the ZEPPELIN-696 branch 3 times, most recently from ce6cd54 to e9510e9CompareFebruary 27, 2016 15:30
@doanduyhai
doanduyhaiforce-pushed the ZEPPELIN-696 branch 2 times, most recently from 1cd7912 to 9d8800fCompareMarch 27, 2016 14:00
@doanduyhai

Copy link
Copy Markdown
ContributorAuthor

@corneadoug@Leemoonsoo

Rebased from master, only JS change, can be reviewed and merged quickly.

Do you want me to add also a test in ZeppelinIT to test for the presence at the notification in case of error ?

@Leemoonsoo

Copy link
Copy Markdown
Member

I have tested and working well.
Looks good to me.

@corneadoug Can you take a look, too?

@Leemoonsoo

Copy link
Copy Markdown
Member

@corneadoug ping

@doanduyhai

Copy link
Copy Markdown
ContributorAuthor

Can we merge this PR so I can move to the next sub-task ?

@doanduyhai

Copy link
Copy Markdown
ContributorAuthor

ping @Leemoonsoo@corneadoug

@corneadoug

Copy link
Copy Markdown
Contributor

@doanduyhai LGTM

@asfgitasfgit closed this in b51af33Apr 4, 2016
onkarshedge pushed a commit to onkarshedge/incubator-zeppelin that referenced this pull request May 11, 2016
### What is this PR for?
Add notification system for AngularJS z functions. Now that we expose the `z` Angular object, we should also catch error/exceptions and display error messages properly to the user.
I changed a little bit the default settings for **ngToast**. Instead of having notifications on **bottom right**, I let the default value undefined.
For **Interpreter** page notification, I force the position to **bottom right** as before when using default config.
For `z` functions error messages, I choose to display notifications on **top right** corner because it's clearer. See screen capture
_This is a sub-task of epic **[ZEPPELIN-635]**_
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Code Review
* [ ] - Simple Test
### Is there a relevant Jira issue?
**[ZEPPELIN-696]**
### How should this be tested?
* `git fetch origin pull/744/head:AngularJSNotification`
* `git checkout AngularJSNotification`
* `mvn clean package -DskipTests`
* `bin/zeppelin-daemon.sh restart`
* Create a new note
* In the first paragraph, put the following code
```html
%angular
<form class="form-inline">
<button type="submit" class="btn btn-primary" ng-click="z.runParagraph()"> Run Paragraph Without Id</button>
<button type="submit" class="btn btn-primary" ng-click="z.runParagraph('20160223-141919_305734436')"> Run Paragraph Without Wrong Id</button>
<button type="submit" class="btn btn-primary" ng-click="z.angularBind('superhero', superhero)"> Angular Bind Without Paragraph Id</button>
<button type="submit" class="btn btn-primary" ng-click="z.angularUnbind('superhero')"> Angular UnBind Without Paragraph Id</button>
</form>
```
* Click on each button to see the appropriate error message
### Screenshots (if appropriate)
![angularnotification](https://cloud.githubusercontent.com/assets/1532977/13901504/734b5112-ee26-11e5-9962-bcf2101eb700.gif)
### Questions:
* Does the licenses files need update? --> **No**
* Is there breaking changes for older versions? --> **No**
* Does this needs documentation? --> **Yes**
[angular-growl-2]: http://janstevens.github.io/angular-growl-2/
[ZEPPELIN-635]: https://issues.apache.org/jira/browse/ZEPPELIN-635
[ZEPPELIN-696]: https://issues.apache.org/jira/browse/ZEPPELIN-696
Author: DuyHai DOAN <doanduyhai@gmail.com>
Closesapache#744 from doanduyhai/ZEPPELIN-696 and squashes the following commits:
0f7bff4 [DuyHai DOAN] [ZEPPELIN-696] Add notification system for AngularJS z functions
Sign up for freeto 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.

3 participants

@doanduyhai@corneadoug@Leemoonsoo