Skip to content

feat(NotificationDrawer): Add notification drawer demo #4640

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.

Sign up for GitHub

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

Merged
tlabaj merged 6 commits into from
Aug 4, 2020
Merged

feat(NotificationDrawer): Add notification drawer demo #4640

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e3745e7
feat(NotificationDrawer): Add notificaiton drawer demo
nicolethoen Jul 29, 2020
c5fd48a
fix snapshots
nicolethoen Jul 31, 2020
abc9496
reorgainize PageHeaderToolItems in demos
nicolethoen Aug 3, 2020
2a06062
address PR comments after incorporating updated notification badge
nicolethoen Aug 3, 2020
33daf88
only apply selected modifier if notifications all read
nicolethoen Aug 3, 2020
42c7fd1
bump version and fix selected icon modifier application
nicolethoen Aug 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react-catalog-view-extension/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"clean": "rimraf dist"
},
"dependencies": {
"@patternfly/patternfly": "4.31.2",
"@patternfly/patternfly": "4.31.3",
"@patternfly/react-core": "^4.38.1",
"@patternfly/react-styles": "^4.6.1",
"classnames": "^2.2.5",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-charts/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
},
"homepage": "https://github.com/patternfly/patternfly-react#readme",
"dependencies": {
"@patternfly/patternfly": "4.31.2",
"@patternfly/patternfly": "4.31.3",
"@patternfly/react-styles": "^4.6.1",
"@patternfly/react-tokens": "^4.8.1",
"hoist-non-react-statics": "^3.3.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
"tslib": "^1.11.1"
},
"devDependencies": {
"@patternfly/patternfly": "4.31.2",
"@patternfly/patternfly": "4.31.3",
"@rollup/plugin-commonjs": "^11.0.2",
"@rollup/plugin-node-resolve": "^7.1.1",
"@rollup/plugin-replace": "^2.3.1",
Expand Down
4 changes: 3 additions & 1 deletion packages/react-core/src/components/NotificationDrawer/NotificationDrawerHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ export const NotificationDrawerHeader: React.FunctionComponent<NotificationDrawe
<Text component={TextVariants.h1} className={css(styles.notificationDrawerHeaderTitle)}>
{title}
</Text>
{count && <span className={css(styles.notificationDrawerHeaderStatus)}>{`${count} ${unreadText}`}</span>}
{count !== undefined && (
<span className={css(styles.notificationDrawerHeaderStatus)}>{`${count} ${unreadText}`}</span>
Copy link
Collaborator

@jschuler jschuler Aug 4, 2020

Choose a reason for hiding this comment

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

this seems a bit too rigid of a format and won't work well with i18n

Copy link
Contributor Author

@nicolethoen nicolethoen Aug 4, 2020

Choose a reason for hiding this comment

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

What do you propose?

Copy link
Collaborator

@jschuler jschuler Aug 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not a blocker if this was already released, but I question if the count prop here is needed, user could just pass <NotificationDrawerHeader unreadText=`${count} unread` />

Copy link
Contributor Author

@nicolethoen nicolethoen Aug 4, 2020

Choose a reason for hiding this comment

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

ah
NotificationDrawerGroup also expects a count: number; to display in its header.

I guess I'm not sure I see a big difference, and it would be a breaking change to remove count from NotificaitonDrawerHeader

Copy link
Collaborator

@jschuler jschuler Aug 4, 2020

Choose a reason for hiding this comment

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

It's a little different in the drawer group because it's just the count there. Here we mandate that count is followed by a space and then a string which is not correct for some languages. This already existed before so not a blocker for this PR tho! But we are accruing technical debt here, since this is a beta component I am not sure if breaking changes are allowed or not.

Copy link
Collaborator

@jschuler jschuler Aug 4, 2020

Choose a reason for hiding this comment

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

#4651

)}
{children && <div className={css(styles.notificationDrawerHeaderAction)}>{children}</div>}
</div>
);
Expand Down
49 changes: 38 additions & 11 deletions packages/react-core/src/components/Page/Page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import styles from '@patternfly/react-styles/css/components/Page/page';
import { css } from '@patternfly/react-styles';
import globalBreakpointXl from '@patternfly/react-tokens/dist/js/global_breakpoint_xl';
import { debounce } from '../../helpers/util';
import { Drawer, DrawerContent, DrawerContentBody, DrawerPanelContent } from '../Drawer';

export enum PageLayouts {
vertical = 'vertical',
Expand Down Expand Up @@ -33,6 +34,12 @@ export interface PageProps extends React.HTMLProps<HTMLDivElement> {
header?: React.ReactNode;
/** Sidebar component for a side nav (e.g. <PageSidebar />) */
sidebar?: React.ReactNode;
/** Notification drawer component for an optional notification drawer (e.g. <NotificationDrawer />) */
notificationDrawer?: React.ReactNode;
/** Flag indicating Notification drawer in expanded */
isNotificationDrawerExpanded?: boolean;
/** Callback when notification drawer panel is finished expanding. */
onNotificationDrawerExpand?: () => void;
/** Skip to content component for the page */
skipToContent?: React.ReactElement;
/** Sets the value for role on the <main> element */
Expand Down Expand Up @@ -73,7 +80,9 @@ export class Page extends React.Component<PageProps, PageState> {
isManagedSidebar: false,
defaultManagedSidebarIsOpen: true,
onPageResize: (): void => null,
mainTabIndex: -1
mainTabIndex: -1,
isNotificationDrawerExpanded: false,
onNotificationDrawerExpand: () => null
};

constructor(props: PageProps) {
Expand Down Expand Up @@ -134,6 +143,9 @@ export class Page extends React.Component<PageProps, PageState> {
children,
header,
sidebar,
notificationDrawer,
isNotificationDrawerExpanded,
onNotificationDrawerExpand,
skipToContent,
role,
mainContainerId,
Expand All @@ -154,22 +166,37 @@ export class Page extends React.Component<PageProps, PageState> {
isNavOpen: mobileView ? mobileIsNavOpen : desktopIsNavOpen
};

const main = (
<main
role={role}
id={mainContainerId}
className={css(styles.pageMain)}
tabIndex={mainTabIndex}
aria-label={mainAriaLabel}
>
{breadcrumb && <section className={css(styles.pageMainBreadcrumb)}>{breadcrumb}</section>}
{children}
</main>
);

const panelContent = <DrawerPanelContent>{notificationDrawer}</DrawerPanelContent>;

return (
<PageContextProvider value={context}>
<div {...rest} className={css(styles.page, className)}>
{skipToContent}
{header}
{sidebar}
<main
role={role}
id={mainContainerId}
className={css(styles.pageMain)}
tabIndex={mainTabIndex}
aria-label={mainAriaLabel}
>
{breadcrumb && <section className={css(styles.pageMainBreadcrumb)}>{breadcrumb}</section>}
{children}
</main>
{notificationDrawer && (
<div className={css(styles.pageDrawer)}>
<Drawer isExpanded={isNotificationDrawerExpanded} onExpand={onNotificationDrawerExpand}>
<DrawerContent panelContent={panelContent}>
<DrawerContentBody>{main}</DrawerContentBody>
</DrawerContent>
</Drawer>
</div>
)}
{!notificationDrawer && main}
</div>
</PageContextProvider>
);
Expand Down
10 changes: 10 additions & 0 deletions packages/react-core/src/components/Page/__tests__/__snapshots__/Page.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ exports[`Check dark page against snapshot 1`] = `
}
id="PageId"
isManagedSidebar={false}
isNotificationDrawerExpanded={false}
mainTabIndex={-1}
onNotificationDrawerExpand={[Function]}
onPageResize={[Function]}
sidebar={
<PageSidebar
Expand Down Expand Up @@ -124,7 +126,9 @@ exports[`Check page horizontal layout example against snapshot 1`] = `
}
id="PageId"
isManagedSidebar={false}
isNotificationDrawerExpanded={false}
mainTabIndex={-1}
onNotificationDrawerExpand={[Function]}
onPageResize={[Function]}
sidebar={
<PageSidebar
Expand Down Expand Up @@ -234,7 +238,9 @@ exports[`Check page to verify breadcrumb is created 1`] = `
}
id="PageId"
isManagedSidebar={false}
isNotificationDrawerExpanded={false}
mainTabIndex={-1}
onNotificationDrawerExpand={[Function]}
onPageResize={[Function]}
sidebar={
<PageSidebar
Expand Down Expand Up @@ -536,8 +542,10 @@ exports[`Check page to verify skip to content points to main content region 1`]
}
id="PageId"
isManagedSidebar={false}
isNotificationDrawerExpanded={false}
mainContainerId="main-content-page-layout-test-nav"
mainTabIndex={-1}
onNotificationDrawerExpand={[Function]}
onPageResize={[Function]}
sidebar={
<PageSidebar
Expand Down Expand Up @@ -834,7 +842,9 @@ exports[`Check page vertical layout example against snapshot 1`] = `
}
id="PageId"
isManagedSidebar={false}
isNotificationDrawerExpanded={false}
mainTabIndex={-1}
onNotificationDrawerExpand={[Function]}
onPageResize={[Function]}
sidebar={
<PageSidebar
Expand Down
Loading