[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
GitHub user doanduyhai opened a pull request:

    https://github.com/apache/incubator-zeppelin/pull/744

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

    ### 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.
   
    One possible solution is to use _growl style_ notification. I use the **[angular-growl-2]** library.
   
   
    _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('xxx')"> 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)
    ![angularjsnotification](https://cloud.githubusercontent.com/assets/1532977/13283917/0bdfd64e-daf1-11e5-9f25-f2c3f526e4e6.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

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/doanduyhai/incubator-zeppelin ZEPPELIN-696

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-zeppelin/pull/744.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #744
   
----
commit f221238b6b0bdff889bc996742ac10348eb4a081
Author: DuyHai DOAN <[hidden email]>
Date:   2016-02-22T16:45:34Z

    [ZEPPELIN-689] Make AngularObject constructor public because of serialization issue

commit bfe1a22aeae9178d4a3bf5e8d53fda525ac5f6cd
Author: DuyHai DOAN <[hidden email]>
Date:   2016-02-22T22:28:11Z

    [ZEPPELIN-689] ZeppelinContext angular() method should look for variable using the paragraph scope then note scope

commit c11b86c6815f510d71490edad311aab0484d2ac5
Author: DuyHai DOAN <[hidden email]>
Date:   2016-02-23T10:11:40Z

    [ZEPPELIN-689] Add Thrift RPC method angularRegistryPush()

commit d81d06a85b14c4704dc5c676301ac8137eabaa0e
Author: DuyHai DOAN <[hidden email]>
Date:   2016-02-23T10:01:31Z

    [ZEPPELIN-689] Implement z.angularBind() function

commit 1592c2966b4c4678f26f09bb794516e1ac8a2475
Author: DuyHai DOAN <[hidden email]>
Date:   2016-02-23T14:20:46Z

    [ZEPPELIN-693] Add AngularJS z.angularUnbind()

commit eeaf137ab5ec08be4d047f34e5787879ac1e649f
Author: DuyHai DOAN <[hidden email]>
Date:   2016-02-23T15:07:36Z

    [ZEPPELIN-695] Add AngularJS z.runParagraph()

commit 95d08c90411dcae3c57ba0bc0a42455b115a4b7d
Author: DuyHai DOAN <[hidden email]>
Date:   2016-02-23T22:33:18Z

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

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
Github user corneadoug commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-188856053
 
    We already have ngtoast for that


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user doanduyhai commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-188981265
 
    Right, didn't know that we had ngToast. I never see it in action. Will remove angular-growl-2


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user doanduyhai commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-202130699
 
    @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 ,


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user Leemoonsoo commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-202155963
 
    I have tested and working well.
    Looks good to me.
   
    @corneadoug Can you take a look, too?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user Leemoonsoo commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-203259282
 
    @corneadoug ping


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user doanduyhai commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-204690146
 
    Can we merge this PR so I can move to the next sub-task ?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user doanduyhai commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-205022696
 
    ping @Leemoonsoo @corneadoug


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user corneadoug commented on the pull request:

    https://github.com/apache/incubator-zeppelin/pull/744#issuecomment-205101210
 
    @doanduyhai LGTM


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] incubator-zeppelin pull request: [ZEPPELIN-696] Add notification s...

yx91490
In reply to this post by yx91490
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-zeppelin/pull/744


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---