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.

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

Custom Notifications #7670

Closed
wants to merge 6 commits into from

Conversation

panda01
Copy link
Contributor

@panda01 panda01 commented Jul 7, 2016

Fixes #6933.

  • Adds a customActions ability to Notifier.
  • Adds a custom function on the notifier that gives granular control to the user.

In order to use these notifications one must do something similar to the following code

import Notifier from 'ui/notify/notifier';
const notify = new Notifier({location: 'AppName'});
notify.custom({
  markdown: 'Some markdown content',
  customActions: [{
    key: 'More Info',
    callback: function() { window.alert('hello world'); }
  }, {
    key: 'Cancel',
    callback: function() { fooCancelBar(); }
  }]
});

All of the callbacks are wrapped in a close function. If you wish to display new information make a new notification.

@tsullivan
Copy link
Member

I got a working example of using this. So far it seems pretty good, and I like that it comes with a small code change.

@ycombinator
Copy link
Contributor

ycombinator commented Jul 9, 2016

@panda01 Thanks for including sample code on how to use this new functionality! Would you mind pasting below that code a screenshot of the notification it generates? It'll just make it super obvious what this awesome functionality does.

@tsullivan
Copy link
Member

I was thinking of commenting that it would be nice to have some kind of spinner or loading icon shown when the action clicked is asynchronous, that we can see while the response is waiting. But that might be overkill. I think we can provide feedback that we're waiting for a response using multiple notifications in sequence.

Using multiple notifications in sequence seems like it will be a common pattern, since each notification can only show a static message. But that should be fine -- I don't think that needs to change.

@panda01 panda01 force-pushed the feature/custom-notifications branch from 9693ffe to 9313102 Compare July 13, 2016 13:19
@panda01 panda01 added the P1 label Jul 13, 2016
*/
Notifier.prototype.custom = function (config) {
const customActionMax = 2;
const mergedConfig = _.assign({}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to assign to an empty object here, you can just mutate the default settings object.

@w33ble
Copy link
Contributor

w33ble commented Jul 13, 2016

I'm a bigger fan of putting the message as a first parameter, with the options following. The message is the only part that's really required, and then you don't leak the markdown property, or have to explain the difference between that and content:

import Notifier from 'ui/notify/notifier';
const notify = new Notifier({location: 'AppName'});
notify.custom('Some markdown *content*', {
  customActions: [{
    key: 'More Info',
    callback: function() { window.alert('hello world'); }
  }, {
    key: 'Cancel',
    callback: function() { fooCancelBar(); }
  }]
});

@tsullivan you're the other person that's going to be using this right away, what do you think?

} else if (notif.customActions) {
// wrap all of the custom functions in a close
notif.customActions = notif.customActions.map(action => {
action.callback = closeNotif(notif, action.callback, action.key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why use key in the definition? From the outside, that parameter is really just defining the button text, isn't it? Couldn't it be text, like:

customActions: [{ text: 'Do it!', callback: doTheThing }]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed I'll change this

@w33ble
Copy link
Contributor

w33ble commented Jul 13, 2016

I thought we were going to make it so that clicking on any of the controls would close the notification, and if you needed a follow-up, your callback could just open a new notification. Right now, I don't think there's any way for the callback to close the notification, it doesn't look like the instance is passed to the callback.

jul-13-2016 15-28-34

It looks like the code should be closing the dialog. Perhaps it's not working as expected?

@panda01
Copy link
Contributor Author

panda01 commented Jul 13, 2016

It looks like the code should be closing the dialog. Perhaps it's not working as expected?

Definitely not working as expected. I'll take a look

@w33ble
Copy link
Contributor

w33ble commented Jul 18, 2016

hey, jenkins, buddy, can you test this please?

* }
*/
Notifier.prototype.custom = function (config) {
const customActionMax = 2;
Copy link
Member

Choose a reason for hiding this comment

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

This should be bumped to 3, so we can have actions like "Sure", "No thanks", and "Maybe later"

@panda01 panda01 removed their assignment Jul 20, 2016
@tsullivan
Copy link
Member

Replacing with #7781

@tsullivan tsullivan closed this Jul 20, 2016
jbudz pushed a commit that referenced this pull request May 10, 2024
`v94.2.1-backport.0` ⏩ `v94.3.0`

_[Questions? Please see our Kibana upgrade
FAQ.](https://github.com/elastic/eui/blob/main/wiki/eui-team-processes/upgrading-kibana.md#faq-for-kibana-teams)_

---

## [`v94.3.0`](https://github.com/elastic/eui/releases/v94.3.0)

- Updated `launch` glyph for `EuiIcon`
([#7670](elastic/eui#7670))
- Updated `EuiComboBox`'s `options` to support including tooltip details
for selectable options. Use `toolTipContent` to render tooltip
information, and `toolTipProps` to optionally customize the tooltip
rendering behavior ([#7700](elastic/eui#7700))
- Updated the following existing glyphs in `EuiIcon`:
([#7727](elastic/eui#7727))
  - `error` (now an outlined version instead of filled) 
  - `tokenMetricCounter`
  - `tokenMetricGauge` 
- Added the following new glyphs to `EuiIcon`:
([#7727](elastic/eui#7727))
  - `tokenDimension`
  - `clickLeft`
  - `clickRight`
  - `clockCounter`
  - `errorFilled` (the previous `error` glyph design)
  - `warningFilled`

**Bug fixes**

- Fixed a visual layout bug for `EuiComboBox` with `isLoading` in mobile
views ([#7700](elastic/eui#7700))
- Fixed missing styles on header cells of `EuiDataGrid` that prevented
content text alignment styles to apply
([#7720](elastic/eui#7720))
- Fixed `EuiFlexGroup` and `EuiFlexItem` `ref` prop typing to support
refs of the same type as the passed `component` type and allow
`displayName` to be defined for easy component naming when using
component wrappers like `styled()`
([#7724](elastic/eui#7724))

---

Most of the code changes you'll see in this PR are caused by the recent
EuiFlex* changes making it generic. This, unfortunately, is something
that `styled()` doesn't always like. I replaced the failing usages of
`styled(EuiFlexGroup)` and `styled(EuiFlexItem)` to use `component` and
other native EuiFlex* props, resulting in the same output but being
better typed.

We plan to add more props to EuiFlex* components giving developers
control over properties like `flex-grow` and `flex-shring`, and reducing
the need for writing any custom CSS when using these components. This
should reduce the number of `styled()` wrappers needed even further

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Template Notifications
4 participants