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

Notification drawer component for console. #3899

Merged
merged 10 commits into from Jan 24, 2020

Conversation

jcaianirh
Copy link
Member

@jcaianirh jcaianirh commented Jan 8, 2020

For jira story: https://issues.redhat.com/browse/CONSOLE-1945

Add Notification Drawer component and add some initial test notifications from the dashboard as well as some fake update alerts.

Includes:

  1. mvp ui design by @cshinn (Add notification drawer MVP designs openshift-origin-design#306)
  2. empty state
  3. link to alert details
  4. read/unread notification bell based on populated alerts/system updates
  5. expand/unexpand based on bell icon
  6. inline drawer panel (pushes content left...option to remove inline to have content overlay. to see an example: https://www.patternfly.org/v4/documentation/react/experimental/drawer#basic-inline)
  7. validate design / implementation with designers
  8. notification badge only visible to users that have roles that allow viewing of alerts
  9. redux store created for notification alerts, and data pulled from that store (fixed flashing alerts and not borrowing dashboard code anymore)

To Do (can be done in follow on pr's):
1. redux work to populate notifications
2. Component updates based on visual inspection
3. add real update notifications
4. responsive strategy with drawer (push content left vs hover over)
5. add warning icon in masthead when alerts present in mobile

6. unit / end to end tests

drawer-empty-gif-new

drawer-gif-new

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. component/core Related to console core functionality labels Jan 8, 2020
@jcaianirh jcaianirh force-pushed the drawer branch 3 times, most recently from 2d69cc2 to a7ffc57 Compare January 9, 2020 18:09
@sg00dwin
Copy link
Member

sg00dwin commented Jan 9, 2020

Posting any issues found that will need to be addressed.

In page sidebar open

drawer1

@benjaminapetersen benjaminapetersen changed the title [WIP]: Notificaion drawer component for console. [WIP]: Notification drawer component for console. Jan 14, 2020
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2020
@jcaianirh jcaianirh changed the title [WIP]: Notification drawer component for console. Notification drawer component for console. Jan 15, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2020
@jcaianirh
Copy link
Member Author

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2020
@openshift openshift deleted a comment from openshift-ci-robot Jan 18, 2020
<DrawerContent>{children}</DrawerContent>
<DrawerPanelContent noPadding>
<NotificationDrawerHeading count={alertData.length + updateData.length}>
<NotificationTypeHeading
Copy link
Contributor

Choose a reason for hiding this comment

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

Type, I ponder if Category is more clear. We don't mean anything that already exists, like a Kind, or a programmatic type. Not a strong feeling atm, just thinking out loud.

}) => {
const [isAlertExpanded, toggleAlertExpanded] = React.useState<boolean>(true);
const [isAvailableUpdateExpanded, toggleAvailableUpdateExpanded] = React.useState<boolean>(false);
const criticalAlerts = _.isEmpty(alertData) ? emptyState(toggleExpanded) : alertData;
Copy link
Contributor

Choose a reason for hiding this comment

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

So Critical Alerts appears to be both Critical and Warning lvl. Is this correct? When I open the drawer in my cluster, I have 18 total Alerts. I have to scroll to find the Critical ones. It does seem like noise pushes the urgent issues blow the fold. For example, I have a lot of UsingDeprecatedAPIAppsV1beta2 warnings, prob don't care a ton about these, but the KubePodCrashLooping I probably do care about:

Screen Shot 2020-01-19 at 10 32 50 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

@benjaminapetersen good point. @cshinn, do we want to display all alerts or just the warning/critical? Currently they are sorted by time...is that correct?

@benjaminapetersen
Copy link
Contributor

Default is Critical Alerts open, which can make it easy to miss the Messages tab entirely.

Screen Shot 2020-01-19 at 10 37 29 PM

Perhaps we would want to save last state in local storage?

@sg00dwin
Copy link
Member

This special case issue displays much better now.

In page sidebar open

drawer1


Screen Shot 2020-01-23 at 12 56 45 PM

@jcaianirh
Copy link
Member Author

thanks @sg00dwin for testing out the drawer interactions. really appreciate it.

@jcaianirh
Copy link
Member Author

/test e2e-gcp-console

@benjaminapetersen
Copy link
Contributor

I just updated, it seems I can get a critical alert on the alerts page but not get it in the drawer:

Screen Shot 2020-01-23 at 3 11 20 PM

This was for:

Screen Shot 2020-01-23 at 3 13 02 PM

Which was generated by running:

oc scale deployment cluster-version-operator --replicas 0 --namespace openshift-cluster-version

@jcaianirh
Copy link
Member Author

@benjaminapetersen had some code in my pocket, just uploaded that fix.

</EmptyStateSecondaryActions>
</EmptyState>
);
const criticalCompare = (a): boolean => getAlertSeverity(a) === 'critical';
Copy link
Contributor

Choose a reason for hiding this comment

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

(a) is any or can we type?

Copy link
Contributor

Choose a reason for hiding this comment

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

(alert: Alert)?

} from '../module/k8s';
import { getSortedUpdates } from './modals/cluster-update-modal';

const emptyState = (toggleExpanded) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

type the callbacks?

const emptyState = (toggleExpanded:() => void) => (

const criticalCompare = (a): boolean => getAlertSeverity(a) === 'critical';
const otherAlertCompare = (a): boolean => getAlertSeverity(a) !== 'critical';

const getAlertNotificationEntries = (
Copy link
Contributor

Choose a reason for hiding this comment

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

const getAlertNotificationEntries = (
  isLoaded: boolean,
  alertData: someStructure?,
  toggleNotificationDrawer: boolean,
  isCritical: boolean,
):  React.ReactNode[] =>

))
: [];

const getUpdateNotificationEntries = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can type as well.

@jcaianirh
Copy link
Member Author

/test analyze

@@ -0,0 +1,262 @@
import * as _ from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

So at this point I would think we want to add new components to frontend/packages/console-app/components rather than public/components. Thinking console-app instead of console-shared as this is an app wide singleton component.

@benjaminapetersen
Copy link
Contributor

@jcaianirh pretty good with where this landed, remaining 2 thoughts:

  • should we move the notification-drawer to packages/console-app per comment above
  • any kind of testing we want to add?

Thanks!

@jcaianirh
Copy link
Member Author

@benjaminapetersen can add unit and or end to end tests as a follow on pr in the testing sprints. not sure on the move to console/app move....we could talk about it in scrum? not sure what the rules are for these two locations. if these are the two final items, lets get this merged for now, and tackle them as next steps. I added the test comments to the description under follow on pr section. all other follow ons listed have been completed.

@benjaminapetersen
Copy link
Contributor

@jcaianirh agree, realizing as I look we have other conventions if we move it (one component per file, etc etc).

Tests can come in the upcoming sprints. Just as good as bug fixes.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, jcaianirh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@jcaianirh
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 56fc64c into openshift:master Jan 24, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants