feat(toastNotification): adds toast notification component by priley86 · Pull Request #48 · patternfly/patternfly-react

priley86 · 2017-08-14T15:53:27Z

Closes #47

What:

adds ToastNotification and ToastNotificationList React component and toast notification stories to Storybook.
fixes some formatting generated by prettier and updates prettier to latest
adds optional dropdownClass to DropdownKebab component
adds TimedToastNotification stateful component for consumers using a timer component

Storybook:
https://rawgit.com/priley86/patternfly-react/toast-notification/index.html

ohadlevy · 2017-08-14T19:39:40Z

src/ToastNotification/TimedToastNotification.js

+  children: PropTypes.node
+}
+TimedToastNotification.defaultProps = {
+  type: 'error',


should error be the default?

I think we did the same with Alert. Happy to change this if you want...I will just make Alert match the same.

ohadlevy · 2017-08-14T19:40:44Z

src/ToastNotification/TimedToastNotification.js

+    const { notificationClass, type, onDismiss, children } = this.props
+    return (
+      <ToastNotification
+        notificationClass={notificationClass}


you can use {...} operator instead of reassigning most of the props.

i'll try this out soon..

ohadlevy

overall looks good code wize, could you please add:

tests
storybook with timer

ohadlevy · 2017-08-14T19:42:05Z

src/ToastNotification/ToastNotification.js

+  return (
+    <div className={notificationClasses}>
+      {onDismiss &&
+        <button


not sure if it matters, but should we use bootstrap Button vs html button?

+1 .. i'll try it

ohadlevy · 2017-08-14T19:42:29Z

src/ToastNotification/ToastNotification.js

+          aria-hidden="true"
+          onClick={onDismiss}
+        >
+          <span className="pficon pficon-close" />


not sure if matters either, but I would prefer to have an Icon component?

i like this idea (have seen it used in other frameworks), but I am only seeing Glyphicons in React Bootstrap. Can you propose one you'd like me to try or maybe file a subsequent PR after this? Also not sure as to the syntax we'd like...

ohadlevy · 2017-08-14T19:43:02Z

src/ToastNotification/ToastNotification.js

+  /** callback when alert is dismissed  */
+  onDismiss: PropTypes.func,
+  /** the type of alert  */
+  type: PropTypes.oneOf(['danger', 'error', 'warning', 'success', 'info'])


DRY: maybe extract to a common const?

hmm not sure if this warrants a shared const, as it is singular to this component? (even though Alert is similar, in my mind they are different components sharing common css). It also conceivable these components change in the future. I'm fine with using a shared const if you want though ;)

ohadlevy · 2017-08-14T19:44:22Z

src/common/Timer.js

@@ -0,0 +1,20 @@
+class Timer {


nitpick: not sure if it makes sense here, but should we in general retain copyright of copy/pasted code?

heh ;) I think this warrants it's own comments now in this file...which could subsequently change here...

My hope is to start collectively making our shared inventions available, to aid in making our work easier to share :)

this is a singularly consumed/piece wise block, not a library consumption. It's also evident that consuming React itself makes our entire solution susceptible to the Facebook BSD+patents license. 😸

priley86 · 2017-08-15T00:54:13Z

I've kept the TimedToastNotification alongside ToastNotification to keep the left-hand nav space available. Maybe later we can try out Storybook chapters?
https://github.com/sm-react/storybook-chapters

There are a few snapshots...what else do you want to test?

jtomasek · 2017-08-15T11:40:56Z

src/ToastNotification/TimedToastNotification.js

+  /** timer delay */
+  timerDelay: PropTypes.number,
+  /** additional notification classes */
+  notificationClass: PropTypes.string,


This prop should be called 'className' which is standard for css classes in React

jtomasek · 2017-08-15T11:56:38Z

src/DropdownKebab/DropdownKebab.js

 import PropTypes from 'prop-types'

-const DropdownKebab = ({ children, id, pullRight }) => {
+const DropdownKebab = ({ dropdownClass, children, id, pullRight }) => {


This is better called 'className'

jtomasek · 2017-08-15T12:17:14Z

src/ToastNotification/TimedToastNotification.js

+  }
+  render() {
+    const { notificationClass, type, onDismiss, children } = this.props
+    return (


If I understand it correctly, you'll need to wrap this in additional div and give it onMouseEnter and onMouseLeave props. Otherwise those won't ever get triggered.

Also we'll need to think about this further as notification pausing should happen for all notifications in the 'notification toaster' - a container of all toast notification. Because in case when 3 notification appear at once, user is not able to pause them all at once by hovering.

'toast-notifications-list-pf' see https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/toast.html

priley86 · 2017-08-15T12:31:38Z

@ohadlevy @jtomasek thanks for the reviews here. i will make updates later today...

priley86 · 2017-08-16T03:09:05Z

@jtomasek @ohadlevy i've made updates per suggestions...

please have a look at the timer example in the updated storybook (see actions logger tab as you are hovering)...it shows that onMouseEnter and onMouseLeave are firing individually now. does this work for you guys?
https://rawgit.com/priley86/patternfly-react/toast-notification/index.html

priley86 · 2017-08-28T13:51:09Z

@jtomasek @ohadlevy i've added a helper ToastNotificationList wrapping div class and ensured we have sticky support. looks good now?

@maryclarke @LHinson @jgiardino are we OK w/ the language/story layout on this one now?

jtomasek · 2017-08-29T10:09:17Z

src/DropdownKebab/DropdownKebab.js

+  const kebabClass = ClassNames(
+    {
+      'dropdown-kebab-pf': true
+    },


can be simplified to
'dropdown-kebab-pf'

jtomasek · 2017-08-29T10:30:21Z

src/ToastNotification/TimedToastNotification.js

+      <ToastNotification
+        {...this.props}
+        onMouseEnter={this.onMouseEnter}
+        onMouseLeave={this.onMouseLeave}


The onMouseEnter and onMouseLeave functions should be run when user hovers on ToastNotificationList rather than each individual ToastNotification. User wants to pause all notifications timers when hovering over ToastNotificationList.

So to achieve this, ToastNotificationList will keep 'paused' boolean in its state and toggle it with it's onMouseEnter and onMouseLeave callbacks.

TimedToastNotification will receive 'paused' prop from ToastNotificationList via passing props to children:

React.Children.map( children, child => React.cloneElement(child, { paused: this.state.paused }) );

TimedToastNotification then updates it's timer in componentWillReceiveProps instead of doing it in onMouseEnter/Leave functions.
With this onMouseEnter/Leave props are not needed in ToastNotification unless we really want to keep it around for flexibility.

thanks for the detailed feedback @jtomasek ... i think this works better (and agree it would be weird to see other alerts dismiss if not hovered while hovering another one). I prefer with leaving the onMouseEnter/Leave available at the ToastNotification level as well as providing onMouseEnter/Leave on the ToastNotificationList (for more fine grained control if desired). i will make this update soon :)

@jtomasek i've made updates to account for this now...

ToastNotificationList now has paused state and propogates this to children TimeToastNotification (ensuring that no other toast notifications will dismiss if a single one is hovered). If any notification is hovered, all notifications will reset their timer.

I am also allowing the consumer to observe onMouseEnter/onMouseLeave on the entire list or on each individual notification. I am aiming for as much flexibility as possible!

jgiardino · 2017-09-06T19:40:35Z

Hey @priley86, I like how you present this component in the storybook. I compared the html in this implementation to the html on patternfly.org, and there are a few things I noticed.

Order of elements in DOM:
The order of elements does not match the current html implementation, which is currently:

div.dropdown-kebab-pf
div.toast-pf-action
span.pficon
contents of message

Whereas this implementation uses the following order:

span.pficon
contents of message
div.dropdown-kebab-pf
div.toast-pf-action

Please switch the order so that div.dropdown-kebab-pf and div.toast-pf-action precede the contents of the message. The order of the text message relative to the actions is important to having the elements wrap as expected when the message is too long.

It doesn’t matter so much what order the icon is in, since it’s displayed absolute and provides no content to screenreaders.

The order in your implementation is actually better, in that both sighted users and screenreader users would get the contents in the same order. And in the future, the html/css is expected to change to use flex, to match the order you have now. But until then, we should use the order that matches the current html.

Dismissible Toasts with Kebab:
The knobs allow the toast notification to display both an actions kebab menu and a close button. However, if the toast notification should be dismissible and include an actions kebab menu, then the action to close the notification should be included in the menu.

Is it possible to build this aspect of the pattern into the implementation of the component so that if a notification is dismissible and has a menu, then the close action is automatically included in the menu? If not, can we modify the behavior of the knobs so that this combination is not possible?

Right now, this documentation detail is cited on the angular documentation for the component, but core design documentation is expected to be updated to match.

Toast header vs toast message:
Can you confirm that part of the text message can be wrapped by a <strong> element? Should this be included as part of the implementation? For example, the angular implementation separates the text into ‘Header’ and ‘Message’, with the Header text automatically being wrapped by <strong>.

priley86 · 2017-09-08T15:17:57Z

thanks for the thorough review here @jgiardino !

I've tried to address each, but let me know if this works for you...

Order of elements in DOM:
I've placed elements in the following order to try to alleviate:

span.pficon
div.dropdown-kebab-pf
div.toast-pf-action
contents of message

The message now comes after the kebabs/actions, but i've left the icon rendered before. Moving the icon after kebabs/actions will come at a tradeoff of rendering the icon within the Notification component vs outside the component and alongside other children. As we have it now, numbers 2,3, and 4 above are rendered as children, and 1 is rendered inline (depending on if onDismiss is provided). I'm not sure moving the close icon out of the component is worth the API simplicity lost...but will leave this up to you! We should probably update the Alert to match this (depending the preference here).

Dismissible Toasts with Kebab:
Of course we can do this 😄 . I've updated the menu to conditionally display "Close" depending on the knob.

Toast header vs toast message:
Added an additional knob for "Header" and "Message", and removed "Label". Works?

waldenraines · 2017-09-08T15:36:35Z

@priley86 do you have any thoughts on breaking out the changes unrelated to toast notifications into separate PRs? I think having PRs that address one specific bug/feature/enhancement will make reviews easier.

priley86 · 2017-09-08T15:44:11Z

@waldenraines i will do my best there ;). I believe there was some common changes associated with this PR. Please defer reviewing my other PRs for now (I will label them WIP to denote). This one is most likely the closest to being ready. Would you mind reviewing it after ListView? I know it's a lot...

waldenraines

Looks good to me, thanks!

jgiardino · 2017-09-12T21:13:35Z

Thanks for the updates, @priley86!

Regarding this comment in your previous post:

Dismissible Toasts with Kebab:
Of course we can do this 😄 . I've updated the menu to conditionally display "Close" depending on the knob.

This is great! However, I still see that the X for close remains in this case. Is it possible to remove the X when both the Dismiss and Menu knobs are selected?

LHinson · 2017-09-13T12:58:42Z

Hey @priley86
What's the history on the sticky functionality? This is not part of patternfly-design so want to make sure to loop back around as necessary. Thanks!

jgiardino · 2017-09-14T13:33:08Z

As a follow up to Leslie's comment, in the Storybook example for Toast Notification List, are we supposed to see the behavior of the toast notification automatically disappearing after eight seconds? These notifications are always displaying.

priley86 · 2017-09-18T20:54:42Z

@jgiardino @LHinson after discussing in the design review today, I will update the sticky notification to say "A persistent notification will remain on the screen until closed.". "Sticky" is not consistent with angular-pf and doesn't imply exactly the same meaning (so will just update the verbiage in the visual presentation of the story).

Also - we are awaiting confirmation on the behavior of hover effect on the notification list in the following upstream design issue:
patternfly/patternfly-design#423

priley86 · 2017-10-02T18:54:45Z

@jgiardino i've updated the Toast Notification List example to allow the notifications to dismiss after the specified delay (8 seconds), and included a "reset notification state" button which resets the timers and the notification state. You can now see that hovering the notifications will pause the dismiss. Let me know if this works!

https://rawgit.com/priley86/patternfly-react/toast-notification/index.html?knob-Label=Danger%20Will%20Robinson%21&knob-Header=Great%20job%21&knob-Message=By%20default%2C%20a%20toast%20notification%27s%20timer%20expires%20after%20eight%20seconds.&knob-Type=success&knob-Dismiss=false&knob-Menu=true&knob-Action=true&selectedKind=ToastNotification&selectedStory=Toast%20Notification%20List&full=0&down=1&left=1&panelRight=0&downPanel=storybooks%2Fstorybook-addon-knobs

priley86 · 2017-10-02T20:12:31Z

will rebase this PR after #55 is merged.

jgiardino · 2017-10-02T20:36:50Z

The updates look good to me! Thanks!!

priley86 · 2017-10-03T20:11:39Z

i've rebased this PR w/ latest after merging #55

Closes patternfly#47

priley86 · 2017-10-10T13:15:58Z

merging this PR as a baseline for notification/notification list. we can iterate in subsequent PRs if necessary.

priley86 requested a review from jtomasek August 14, 2017 15:58

priley86 force-pushed the notification branch from e02bebd to 84c4f0a Compare August 14, 2017 19:27

priley86 requested a review from ohadlevy August 14, 2017 19:29

ohadlevy reviewed Aug 14, 2017

View reviewed changes

ohadlevy suggested changes Aug 14, 2017

View reviewed changes

jtomasek suggested changes Aug 15, 2017

View reviewed changes

priley86 force-pushed the notification branch from 84c4f0a to 1e6b509 Compare August 16, 2017 03:04

priley86 force-pushed the notification branch from 1e6b509 to 192b8a7 Compare August 28, 2017 13:49

priley86 force-pushed the notification branch 2 times, most recently from 10316b2 to 01c8dd9 Compare August 28, 2017 20:45

jtomasek reviewed Aug 29, 2017

View reviewed changes

priley86 force-pushed the notification branch 3 times, most recently from 1557322 to 65229ef Compare September 8, 2017 15:05

waldenraines approved these changes Sep 8, 2017

View reviewed changes

priley86 force-pushed the notification branch from 65229ef to ad6bf88 Compare September 18, 2017 17:23

priley86 force-pushed the notification branch from ad6bf88 to 1f38900 Compare October 2, 2017 18:45

priley86 force-pushed the notification branch from 1f38900 to 8667b18 Compare October 2, 2017 18:50

priley86 force-pushed the notification branch 2 times, most recently from 9415be9 to 15d5c30 Compare October 3, 2017 20:09

priley86 force-pushed the notification branch 2 times, most recently from 71685a7 to 9f43445 Compare October 9, 2017 13:14

feat(toastNotification): adds toast notification component

ea97be0

Closes patternfly#47

priley86 force-pushed the notification branch from 9f43445 to ea97be0 Compare October 9, 2017 13:15

priley86 merged commit befe111 into patternfly:master Oct 10, 2017

Conversation

priley86 commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ohadlevy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 commented Aug 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 commented Aug 15, 2017

Uh oh!

priley86 commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 commented Aug 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priley86 Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

priley86 commented Sep 8, 2017

Uh oh!

waldenraines commented Sep 8, 2017

Uh oh!

priley86 commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

waldenraines left a comment

Choose a reason for hiding this comment

Uh oh!

jgiardino commented Sep 12, 2017

Uh oh!

LHinson commented Sep 13, 2017

Uh oh!

priley86 commented Aug 14, 2017 •

edited

Loading

priley86 Aug 15, 2017 •

edited

Loading

priley86 commented Aug 15, 2017 •

edited

Loading

priley86 commented Aug 16, 2017 •

edited

Loading

priley86 Aug 29, 2017 •

edited

Loading

priley86 Sep 8, 2017 •

edited

Loading

jgiardino commented Sep 6, 2017 •

edited

Loading

priley86 commented Sep 8, 2017 •

edited

Loading

priley86 commented Sep 18, 2017 •

edited

Loading

priley86 commented Oct 2, 2017 •

edited

Loading