-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Adds SuitesReminder hook to get a consistent look across platforms for the reminder notification #221
Changes from 7 commits
ea4fbba
6b35c3b
973fe4d
13393bc
fd97683
36cea5f
273bcba
7683342
01f5097
6f8269e
f384697
a6746d0
b75ec62
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.globalNavigation__suitesReminder.globalNavigation__suitesReminder-bottomLeft { | ||
jared-dickman marked this conversation as resolved.
Show resolved
Hide resolved
|
||
left: calc(var(--nav-width) + var(--margin-xs)) !important; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import 'src/styles/_variables.css' | ||
import './suites-reminder.css' | ||
|
||
tibuurcio marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import { type ReactNode } from 'react' | ||
import { Button, notification, Space } from 'src/components' | ||
import { FontWeightStrong } from 'src/styles/style' | ||
|
||
export interface ISuitesReminderOptions { | ||
onClose: () => void | ||
onRemindMeLater: () => void | ||
onTakeMeThere: () => void | ||
duration?: number | ||
title?: string | ||
message?: string | ||
} | ||
|
||
type OpenNotificationFn = () => void | ||
type ContextHolder = ReactNode | ||
|
||
export type SuitesReminderHook = [OpenNotificationFn, ContextHolder] | ||
|
||
const DefaultReminderDuration = 4.5 // same as antd notification default duration | ||
tibuurcio marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const DefaultTitle = 'Join the new mParticle Experience!' | ||
const DefaultMessage = | ||
'Managing your data is easier than ever with the new mParticle experience. Try out the latest features with ease, and switch back to the classic experience anytime from the side navigation.' | ||
|
||
export const useSuitesReminder = (options: ISuitesReminderOptions): SuitesReminderHook => { | ||
const { | ||
onClose, | ||
onRemindMeLater, | ||
onTakeMeThere, | ||
duration = DefaultReminderDuration, | ||
title = DefaultTitle, | ||
message = DefaultMessage, | ||
} = options | ||
|
||
const [notificationApi, contextHolder] = notification.useNotification({ | ||
prefixCls: 'globalNavigation__suitesReminder', | ||
duration, | ||
placement: 'bottomLeft', | ||
}) | ||
|
||
const openNotification = (): void => { | ||
const key = `notification-${Date.now()}` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be a bit of a nit on my part, but I think using date conversion for a key might be overkill and might have a performance hit. Why not just create a simple increment function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting. Haven't given much thought to it but am always concerned with performance. I think in this case we will be fine since it will never be the case of showing more than just a few notifications at a time. Also it's the way Ant suggests in their docs. |
||
const btn = ( | ||
<Space> | ||
<Button | ||
type="link" | ||
size="small" | ||
onClick={_event => { | ||
onRemindMeLater() | ||
notificationApi.destroy(key) | ||
}}> | ||
Remind me later | ||
</Button> | ||
<Button | ||
type="primary" | ||
size="small" | ||
onClick={_event => { | ||
onTakeMeThere() | ||
notificationApi.destroy(key) | ||
}}> | ||
Take me there | ||
</Button> | ||
</Space> | ||
) | ||
|
||
notificationApi.open({ | ||
message: <span style={{ fontWeight: FontWeightStrong }}>{title}</span>, | ||
description: message, | ||
btn, | ||
key, | ||
onClose, | ||
}) | ||
} | ||
|
||
return [openNotification, contextHolder] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
would be a good place to write some tests too 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh you're so right. I totally forget about it. Will try some 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!