Add Alert Group demo by tompsota · Pull Request #7843 · patternfly/patternfly-react

tompsota · 2022-08-17T10:54:49Z

What: Closes #7008

patternfly-build · 2022-08-17T11:06:29Z

Preview: https://patternfly-react-pr-7843.surge.sh

A11y report: https://patternfly-react-pr-7843-a11y.surge.sh

wise-king-sullyman · 2022-08-18T18:26:45Z

I believe that the main issue here is actually rooted in AlertGroupInline using the index of each child as the key for the list items it creates. Changing the key to be based on the title of the Alert (and adding a key to each Alert created in the example) rather than the index seems to make things timeout properly in my testing.

The problem with that specific approach is that the title isn't going to necessarily be a string since it's typed as a ReactNode, though I'm not sure yet what would be more appropriate.

tompsota · 2022-08-22T16:34:52Z

I believe that the main issue here is actually rooted in AlertGroupInline using the index of each child as the key for the list items it creates. Changing the key to be based on the title of the Alert (and adding a key to each Alert created in the example) rather than the index seems to make things timeout properly in my testing.

The problem with that specific approach is that the title isn't going to necessarily be a string since it's typed as a ReactNode, though I'm not sure yet what would be more appropriate.

Thanks! This was definitely one of the reasons. Maybe it will be worth creating a separate issue where we could discuss what could be used instead of index. I can do that tomorrow.

If you are curious, the problem was a bit trickier in the demo, because I was tying to remove alert with onTimeout prop (which was already done when timeout was over, causing all other alerts to disappear).

wise-king-sullyman · 2022-08-22T22:34:06Z

I went ahead and made an issue for the index bug #7877 and put up a PR #7878 (draft for now because I want to add a test when I get a moment).

I may not be understanding your issue properly.

I pulled your branch and as with my smaller scale testing fixing the index issue (and making a couple of corresponding changes in your demo, you can see them on this branch) seems to fix the issue of the alerts timing out in the wrong order / at the wrong times.

If you could try making the same changes I did and let me know what the others issue are that I'm missing I can take a look into them 🙂

tompsota · 2022-08-23T09:39:44Z

Soo I compared your branch with mine and found out that the other problem was in removeAlert function in AlertGroupDemo.tsx. When I changed that function in your code to following (this is the approach I originally used):

  const removeAlert = (key: React.Key) => {
    setAlerts(alerts.filter(alert => alert.props.uniqueId !== key.toString()));
  };

then all alerts were dismissed at once. Just me being curious, could you explain why this approach doesn't work? I didn't find any particular reason for that.

wise-king-sullyman · 2022-08-23T15:02:35Z

From my understanding of things, I believe what's happening here is that with your original approach removeAlert is getting passed to Alert with the value of alerts as it is at the time that the alert is created, so when the function runs the first time it will still have an alerts value of [], which [].filter then of course returns an empty array and all the alerts are removed.

When using the functional update form in the setAlerts call I am fairly sure that React (through some form of React internal magic) actually gets the current value for alerts as part of the setAlerts call.

I might not be 100% right with this explanation as the react docs aren't super clear about it (they just say to use the functional update form if your new state is dependent on the current state, but don't say why) and when going through blog posts and SO threads there's lots of conflicting "information" about the topic, but this explanation is what makes the most sense to me based on everything I've read and my own testing 😅

Signed-off-by: Tomas Psota <tpsota@redhat.com>

thatblindgeye

This is looking great so far! 🎉 I had some comments below (some a little more nitpicky than others, so let me know what you think), but awesome job working on this!

wise-king-sullyman

I think this is looking really great! I just have a couple of really small nits and one comment that would help DRY things up a touch 🙂

tompsota · 2022-09-12T08:52:12Z

I noticed that the combination of Alert Group with overflow and having the most recent alert on top causes the timeout of alerts that are 'hidden' to start just after they appear on screen.

We discussed this with Austin and we'd like to hear also the opinion from @mcarrano

We found two possible solutions:

This behavior is okay, because every alert should be displayed on the screen for n seconds (where n is the length of timeout). So the older alerts will appear just after the most recent ones timeout. Also this solution is the easiest, because it doesn't require any further modifications.
The timeout should be active also when the alert is 'hidden'. This solution will most likely require us to handle the timeouts outside the alerts - they will be handled in the demo itself. The issue with this approach would be that we don't use the timeout functionality of the Alert but add whole new timeout handling.

Personally, I would prefer the first option. IMO, the second one kind of goes against the purpose of the demo - it should showcase what can be built with the components. When we add add custom timeout handling, then we would showcase just something tailored to the demo instead of the functionality of the Alert.

The example below shows that the oldest success alert is pushed out to the overflow and its timeout starts just after it is again displayed on the screen, after other (newer) alerts disappear. The timeout is 7 seconds.

alerts_behavior.mov

mcarrano · 2022-09-12T13:35:58Z

@tompsota @wise-king-sullyman I think the suggestion to leave this as it is now is fine. I admit that it is a bit odd, but in reality I don't think you'd want to use this overflow feature together with alerts that time out. The purpose of this is more to manage a queue of alerts that you want the user to read and dismiss manually. Maybe the demo should be set up that way.

BTW, I don't seem to be seeing this demo when I preview the PR. Shouldn't this be showing on a demo tab?

thatblindgeye · 2022-09-12T14:38:53Z

BTW, I don't seem to be seeing this demo when I preview the PR. Shouldn't this be showing on a demo tab?

@mcarrano currently the demo is in the "Demos" section of the side-nav.

tompsota · 2022-09-13T10:45:57Z

BTW, I don't seem to be seeing this demo when I preview the PR. Shouldn't this be showing on a demo tab?

@mcarrano currently the demo is in the "Demos" section of the side-nav.

Moved the demo to the demo tab

thatblindgeye

Looking great! I had a couple additional comments below (one nit that I won't block over and one that might be more a CSS issue). I think I'd agree with Matt's comment above about the demo showing alerts that are dismissed manually rather than via timeout, but that isn't really a blocker for me.

thatblindgeye · 2022-09-13T13:46:05Z

+
+## Demos
+
+This demonstrates how you can assemble a full page view including the use of alert group toast notifications with timeout that are also displayed inside the notification drawer.


This could be moved below the example title heading, but since this is the only demo right now I'd also be fine with it being kept where it is.

thatblindgeye · 2022-09-13T13:56:26Z

+          position={DropdownPosition.right}
+        />
+      </NotificationDrawerHeader>
+      <NotificationDrawerBody>


@mcoker currently the last alert in the NotificationDrawerBody causes and overflow when its kebab toggle is opened, even if there's enough vertical room for the kebab dropdown:

Either disabled the overflow-y: auto property or adding a height property to pf-c-notification-drawer__body resolves it (I only added a height: 100% to the element to test quickly). I haven't tested with any flex properties , though (since the body element is in the pf-c-notification-drawer flex container).

Do you think this would be an update that could be made in Core, or maybe just a style specific to this demo?

@thatblindgeye nice catch! That looks like a bug in core to me - patternfly/patternfly#5091

mcoker

LGTM! Left some comments, but I don't think any are blockers.

mcoker · 2022-09-14T15:29:43Z

+
+  return (
+    <DashboardWrapper
+      header={<DashboardHeader notificationBadge={notificationBadge} />}


This may be out of scope, but if you've passed notificationDrawer, I'm wondering if that could be used to create the notification badge automatically and you may be able to remove this? It would be great to make the dashboard wrapper handle showing the notification badge any time there is a notification drawer (and don't show it if there isn't a notification drawer), and ideally handle showing the notification badge state consistently.

@mcoker That definitely is possible and I agree that it would be more consistent.. However, I cannot think about any neat solution. We could access the NotificationDrawerListItems through children of notificationDrawer and extract the required data from there, but that approach requires to go quite deep into the tree and there won't be any guarantee that the required props (variant, isRead) are set. I'd suggest to leave this as it is for now and maybe open this in a new issue, @thatblindgeye what do you think?

I haven't yet tried looking into it more closely, but I would agree that this would make sense as a follow-up issue.

mcoker · 2022-09-14T15:39:04Z

+import { useEffect } from 'react';
+import BellIcon from '@patternfly/react-icons/dist/js/icons/bell-icon';
+import SearchIcon from '@patternfly/react-icons/dist/js/icons/search-icon';
+import DashboardWrapper from './examples/DashboardWrapper';
+import DashboardHeader from './examples/DashboardHeader';


This is probably fine, I just wanted to ask if these need to be imported here since they're also imported in AlertGroupToastWithNotificationDrawer.tsx?

They need to be imported also here, we would get ReferenceError otherwise

tompsota · 2022-09-15T11:25:04Z

I think I'd agree with Matt's comment above about the demo showing alerts that are dismissed manually rather than via timeout, but that isn't really a blocker for me.

@thatblindgeye I don't have any problem with removing timeout from the alerts. When I was implementing this demo, I followed the design doc which mentions that the alerts should have the timeout set to 8 seconds. Here is the design doc, it is mentioned in the 'Behavior' section

wise-king-sullyman

Awesome work on this! 🔥

thatblindgeye

@tompsota regarding your latest reply, since it follows the design guidelines I'm fine with it as-is, and would defer to Matt/design regarding whether the demo should have timeouts removed as part of this PR or if it's something to circle back to at some point.

Echoing above, awesome work! 🎉

mcarrano · 2022-09-15T13:21:17Z

@tompsota @thatblindgeye I'm also fine to merge this as is. Maybe we should reconsider what the design recommendation is for this. I'll queue that up for some discucssion, but no reason to hold this up further.

mcoker

LGTM!

mcarrano

Thanks for making the updates @tompsota . Looks great!

tlabaj

LGTM! great work.

patternfly-build · 2022-09-20T18:29:47Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.80.3
@patternfly/react-catalog-view-extension@4.92.3
@patternfly/react-charts@6.94.3
@patternfly/react-code-editor@4.82.3
@patternfly/react-console@4.92.3
@patternfly/react-core@4.241.3
@patternfly/react-docs@5.102.3
@patternfly/react-icons@4.92.3
@patternfly/react-inline-edit-extension@4.86.3
demo-app-ts@4.201.3
@patternfly/react-integration@4.203.3
@patternfly/react-log-viewer@4.86.3
@patternfly/react-styles@4.91.3
@patternfly/react-table@4.110.3
@patternfly/react-tokens@4.93.3
@patternfly/react-topology@4.88.3
@patternfly/react-virtualized-extension@4.88.3
transformer-cjs-imports@4.79.3

Thanks for your contribution! 🎉

tompsota added the Do Not Merge label Aug 17, 2022

tompsota requested a review from wise-king-sullyman August 17, 2022 10:54

wise-king-sullyman mentioned this pull request Aug 23, 2022

fix(AlertGroup): update AlertGroupInline to not use child index as a key #7878

Merged

tompsota and others added 6 commits September 2, 2022 15:24

added alert group demo

2bc1647

Signed-off-by: Tomas Psota <tpsota@redhat.com>

added alert group demo

e514ec1

Signed-off-by: Tomas Psota <tpsota@redhat.com>

split alerts and notifications

2b2549c

Signed-off-by: Tomas Psota <tpsota@redhat.com>

use title as AlertGroup key

4a83c98

refactor demo

9e38bc3

tmp

126ea48

tompsota force-pushed the add_alertgroup_demo branch from 00744db to 02ad5de Compare September 2, 2022 14:19

remove dismissed alerts counter

3f3a047

tompsota force-pushed the add_alertgroup_demo branch from 02ad5de to 3f3a047 Compare September 2, 2022 16:51

thatblindgeye requested changes Sep 7, 2022

View reviewed changes

wise-king-sullyman reviewed Sep 9, 2022

View reviewed changes

Comment thread packages/react-core/src/demos/examples/AlertGroup/AlertGroupDemo.tsx Outdated

Comment thread packages/react-core/src/demos/examples/AlertGroup/AlertGroupDemo.tsx Outdated

Comment thread packages/react-core/src/demos/examples/AlertGroup/AlertGroupDemo.tsx Outdated

code improvements and refactoring

69e08e8

tompsota removed the Do Not Merge label Sep 13, 2022

thatblindgeye reviewed Sep 13, 2022

View reviewed changes

mcoker reviewed Sep 14, 2022

View reviewed changes

Comment thread packages/react-core/src/demos/examples/AlertGroup/AlertGroupToastWithNotificationDrawer.tsx Outdated

mcoker approved these changes Sep 14, 2022

View reviewed changes

wise-king-sullyman approved these changes Sep 15, 2022

View reviewed changes

thatblindgeye approved these changes Sep 15, 2022

View reviewed changes

thatblindgeye added the Do Not Merge label Sep 19, 2022

tompsota removed the Do Not Merge label Sep 19, 2022

use Button component, add attention badge variant

971aebf

tompsota force-pushed the add_alertgroup_demo branch from 84585c9 to 971aebf Compare September 19, 2022 15:26

mcoker approved these changes Sep 20, 2022

View reviewed changes

mark notifications as read on click

e217344

mcarrano approved these changes Sep 20, 2022

View reviewed changes

tlabaj reviewed Sep 20, 2022

View reviewed changes

tlabaj merged commit c789143 into patternfly:main Sep 20, 2022


		## Demos

		This demonstrates how you can assemble a full page view including the use of alert group toast notifications with timeout that are also displayed inside the notification drawer.

Conversation

tompsota commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wise-king-sullyman commented Aug 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tompsota commented Aug 22, 2022

Uh oh!

wise-king-sullyman commented Aug 22, 2022

Uh oh!

tompsota commented Aug 23, 2022

Uh oh!

wise-king-sullyman commented Aug 23, 2022

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wise-king-sullyman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tompsota commented Sep 12, 2022

Uh oh!

mcarrano commented Sep 12, 2022

Uh oh!

thatblindgeye commented Sep 12, 2022

Uh oh!

tompsota commented Sep 13, 2022

Uh oh!

thatblindgeye left a comment

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Sep 13, 2022 • 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!

thatblindgeye Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

mcoker Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcoker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcoker Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

tompsota Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

thatblindgeye Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcoker Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

tompsota Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

tompsota commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

tompsota commented Aug 17, 2022 •

edited

Loading

patternfly-build commented Aug 17, 2022 •

edited

Loading

wise-king-sullyman commented Aug 18, 2022 •

edited

Loading

thatblindgeye Sep 13, 2022 •

edited

Loading

tompsota commented Sep 15, 2022 •

edited

Loading