Skip to content

Conversation

@Madhuka
Copy link
Contributor

@Madhuka Madhuka commented Aug 28, 2015

Adding Notifications Service for Zeppelin

  • Notifications feature added
    • UI-Notifications component added
    • Zeppelin notifications can be configured
    • Different level of notification
    • ngToast.danger('message')
    • ngToast.warning('message')
    • ngToast.info('message')
    • ngToast.success('message')

Screen shots

screenshot from 2015-08-28 23 45 41

@felixcheung
Copy link
Member

looks good... what is the guideline on when to show this notification?

@Madhuka
Copy link
Contributor Author

Madhuka commented Aug 29, 2015

zeppelin Notifications are designed to inform users about status of zeppelin.
(All Notifications messages should give informative feedback.)

  1. Error Notifications will inform system issues.
    eg: Network issue, File loading issue, System failtures
  2. Warning Notifications are based on incorrect user interactions such as wrong data types/ queries/ wrong data model for chart types.
  3. Primary notifications inform basic informations to the users. There are few other notification levels as well.

Default message will be shown for 10000 ms (time is configurable) and after that the message starts fading out. If user noticed the notification, user can remove it by clicking on it.

@corneadoug
Copy link
Contributor

Just tested it out a bit.
I'm not a big fan of the hover effect on the error (it gets a bit transparent)

I also think the message time should be reduced to something like 6000ms, currently its too long.
I'm not getting the console.log anymore when the error is showing.
(Need to check it out, because we can't have those errors not showing in console)

I think it could also be nice not to include any error call in this PR (just the service) and add error calls in another PR.
Because in current Zeppelin state, there is a lot of problems making the error spam a lot, for example:

  • I get the angular error everytime I hide/show the editor code.
  • I get the angular error 3 times when running the paragraph (because bind is called 3 times)

@Madhuka
Copy link
Contributor Author

Madhuka commented Aug 31, 2015

@corneadoug Thanks for the feedback I will update PR

@djoelz
Copy link

djoelz commented Aug 31, 2015

also what if I want to copy the error? If I click it disappears. Maybe a close X or something and the text is selectable?

@corneadoug
Copy link
Contributor

@djoelz this could be useful.
But I guess this type of error is only as a visual signal that something went wrong and message would be pretty short, you would usually find a stack in the console.

@djoelz
Copy link

djoelz commented Sep 1, 2015

@corneadoug think of a case where the user does not have access to the console. Think Zeppelin hosted as a website. Then users might run into issues that they want who ever is hosting the site to investigate. How would they send these errors? That's why I think being able to copy the text would be useful.

@corneadoug
Copy link
Contributor

console = web browser dev tools console.
What I mean is that those message are visual feedback, they wouldn't be enough for debug.
But copy pasting it could be a plus.

@Madhuka
Copy link
Contributor Author

Madhuka commented Sep 1, 2015

@djoelz Current Zeppelin doesn't have facility to show error messages or system failures for the users on web user interface. So the notification service will enable this. Zeppelin developers could use this notification service for error handling. The answers corneadoug have given are correct.
If any more uncleared issues please let us know.

Thanks.

@Madhuka
Copy link
Contributor Author

Madhuka commented Sep 1, 2015

@corneadoug
Hover is fixed for less transparent and Error Notifications have removed so PR only contain notification services.

Default message time is 6000ms.If notification message is needed to show for more duration than default time. It can be done as below.

  // Message with custom duration 
  Notification.error({message: 'Error notification', delay: 1000});

@Madhuka
Copy link
Contributor Author

Madhuka commented Sep 1, 2015

@Leemoonsoo : Ready for Merge
@bzz, @corneadoug This contain notification service

@corneadoug
Copy link
Contributor

I have more feedbacks/changes, especially to match @djoelz comments.
So I will do a PR on your branch :)

@Madhuka Madhuka changed the title ZEPPELIN-269 : Adding Error Msg and Notifications feature ZEPPELIN-269 : Adding Notifications Service for Zeppelin Sep 1, 2015
@corneadoug
Copy link
Contributor

@Madhuka Can you update the PR description with new screenshot from my PR?

@Madhuka
Copy link
Contributor Author

Madhuka commented Sep 1, 2015

@corneadoug Thanks
I will share it

@Madhuka
Copy link
Contributor Author

Madhuka commented Sep 1, 2015

Moved to 'ngToast Directive' from 'angular-ui-notification'. It include below features

  • Button to close the notification
  • Auto close at 6000ms
  • No close event on notification click
  • Changed the original ngToast style
  • Error message can be copied

Notifications can be used by including ngToast service in the controller. It support four different level of notification:

  • ngToast.danger('message')
  • ngToast.warning('message')
  • ngToast.info('message')
  • ngToast.success('message')

notification-ui

@corneadoug
Copy link
Contributor

Looks good to me (obviously) :)
Merge +1

@djoelz
Copy link

djoelz commented Sep 1, 2015

That looks great! Good job guys!

@Madhuka
Copy link
Contributor Author

Madhuka commented Sep 1, 2015

It is good to go for Merge.
+1

@Leemoonsoo
Copy link
Member

I wanted to mention that there were similar effort before. #224.

Can someone tell me how i can use this feature?

@Madhuka
Copy link
Contributor Author

Madhuka commented Sep 2, 2015

@Leemoonsoo
Yes, #224 also had same feature but in here we some improvements you can see it UI.

You can used notification service by calling.

//passing danger notification for user
ngToast.danger('message')

//passing warning for user
ngToast.warning('message')

//passing some information for user
ngToast.info('message')

//giving success notification for user
ngToast.success('message')

@Leemoonsoo
Copy link
Member

Thanks for usage. tested and working well.
LGTM!

@corneadoug
Copy link
Contributor

#224 was closed, and I'm not sure why.

You can find most of the differences in this PR thread, main ones being:

  • Copy paste of message allowed (no more close notification when clicked)
  • Closing button on the notification

This PR only brings the Notification Service, so that we separate adding the notifications in the zeppelin code from adding the Notification Service.

On top of what @Madhuka said, you will also have to inject the service in your controller, for example:

angular.module('zeppelinWebApp')
  .controller('ParagraphCtrl', function($scope, $rootScope, $route, $window, $element, $routeParams, $location, $timeout, $compile, websocketMsgSrv, ngToast) {

@asfgit asfgit closed this in 706755a Sep 3, 2015
Leemoonsoo pushed a commit to Leemoonsoo/zeppelin that referenced this pull request Sep 17, 2015
### Adding Notifications Service for Zeppelin
 - [x] Notifications feature added
  - [x] UI-Notifications component added
- [x] Zeppelin notifications can be configured
- [x] Different level of notification
    *  ngToast.danger('message')
    *   ngToast.warning('message')
    *   ngToast.info('message')
    *   ngToast.success('message')

## Screen shots
![screenshot from 2015-08-28 23 45 41](https://cloud.githubusercontent.com/assets/1180128/9553799/116e685e-4ddf-11e5-95c3-3fd1fc31b8cd.png)

Author: madhuka <madhukaudantha@gmail.com>
Author: Damien Corneau <corneadoug@gmail.com>

Closes apache#263 from Madhuka/errormsg-services and squashes the following commits:

6d5cf4d [madhuka] Merge pull request #1 from corneadoug/change/errorMsgDirective
3ece1eb [Damien Corneau] configure and style ngToast
460d364 [Damien Corneau] Remove angular-ui-notification for ngToast
35d7469 [madhuka] fixing hover, delay time and cleaning error msg
9b131f2 [madhuka] using bower to update
a0baf57 [madhuka] Adding Error Msg and Notifications feature

(cherry picked from commit 706755a)
Signed-off-by: Lee moon soo <moon@apache.org>
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