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.

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

Simplify KaizenProvider #5426

Merged
merged 12 commits into from
Jan 7, 2025
Merged

Simplify KaizenProvider #5426

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a9eb40a
Simplify KaizenProvider
vietanhtran16 Dec 23, 2024
bc6f449
changeset
vietanhtran16 Dec 23, 2024
9605a5b
Fix story
vietanhtran16 Dec 23, 2024
032f7d8
Fix order import
vietanhtran16 Dec 23, 2024
a131c0a
Merge branch 'main' into viet/debug-kaizen-provider
vietanhtran16 Jan 6, 2025
f7bea24
Revert change to stories and ensure that container is set
vietanhtran16 Jan 6, 2025
dba3278
Replicate the issue
vietanhtran16 Jan 7, 2025
bff6276
Good ol js to create element
vietanhtran16 Jan 7, 2025
8afcee7
Revert changes in story
vietanhtran16 Jan 7, 2025
893ae1e
Merge branch 'main' into viet/debug-kaizen-provider
vietanhtran16 Jan 7, 2025
ff94155
Refactor
vietanhtran16 Jan 7, 2025
ce636f3
Update changeset
vietanhtran16 Jan 7, 2025
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
5 changes: 5 additions & 0 deletions .changeset/proud-cameras-lay.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@kaizen/components': patch
---

Simplify KaizenProvider
6 changes: 2 additions & 4 deletions packages/components/src/KaizenProvider/KaizenProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,19 @@ export type KaizenProviderProps = {

export const KaizenProvider = ({ children, locale = 'en' }: KaizenProviderProps): JSX.Element => {
const [documentIsAvailable, setDocumentIsAvailable] = useState<boolean>(false)
const [notificationsList, setNotificationsList] = useState<JSX.Element>()

useEffect(() => {
// SSR does not have a document, which is required for ToastNotificationsList.
// Await document render before rendering the component.
if (document !== undefined) {
setNotificationsList(<ToastNotificationsList />)
setDocumentIsAvailable(true)
}
}, [documentIsAvailable])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't need to be a dependency, nothing inside useEffect is referring to it

}, [])

return (
<OptionalIntlProvider locale={locale}>
<ToastNotificationProvider>
{notificationsList}
{documentIsAvailable && <ToastNotificationsList />}
{children}
</ToastNotificationProvider>
<FontDefinitions />
Expand Down
37 changes: 19 additions & 18 deletions packages/components/src/Notification/ToastNotification/_docs/ToastNotification.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, { useEffect, useId } from 'react'
import { type Meta, type StoryObj } from '@storybook/react'
import { expect, within } from '@storybook/test'
import { expect, userEvent, within } from '@storybook/test'
import { Button } from '~components/Button'
import { ToastNotification, useToastNotification } from '../index'

Expand Down Expand Up @@ -175,26 +175,27 @@ export const NoDuplicatesWithSameId: Story = {
render: () => {
const { addToastNotification } = useToastNotification()

useEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this story was failing before I made this change due to the toast was not visible.

Not sure why that was the case but I think using useEffect here might have cause some issue as I suspect addToastNotification is not being wrapped inside a useCallBack so having it inside useEffect dependency will cause it to be called many times

addToastNotification({
id: 'id--clear-example-1',
title: 'First',
type: 'positive',
message: 'There should only be one notification',
})
addToastNotification({
id: 'id--clear-example-1',
title: 'First',
type: 'positive',
message: 'There should only be one notification',
})
}, [addToastNotification])

return <div>Irrelevant content</div>
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

The main issue I'd have with you changing this test is that I think some teams actually use addToastNotification in a useEffect in a similar fashion to this. If it fails here, then it will start throwing errors for consumers too.

I suggest you make a canary and test against performance-review-cycles-ui and see if everything passes or not.
Slack thread of the SSR issue when ToastNotificationProvider was introduced: https://cultureamp.slack.com/archives/C0189KBPM4Y/p1700782380804669

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me test it out. I reckon all these addToastNotification updateToastNotification removeToastNotification clearToastNotifications functions should be wrapped inside an useCallback tbh as they can be used as dependency in useEffect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually I will need to test a canary from 1.67.0 version instead as the latest version has some issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

read through your comment here about what the [] dependency array which will cause the issue that I am seeing with this story hence trying to fix it.

That is weird though as documentIsAvailable is not even being used inside useEffect - having it in the dependency array will just cause the useEffect to run again after setDocumentIsAvailable(true) has been called

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always get this error with pnpm storybook even after pnpm reset so unable to run locally and test it
image

<Button
label="Create notification"
onClick={() =>
addToastNotification({
id: 'id--clear-example-1',
title: 'First',
type: 'positive',
message: 'There should only be one notification',
})
}
/>
)
},
play: async (context) => {
const { canvasElement } = context
const { findAllByText } = within(canvasElement.parentElement!)
const { findAllByText, findByRole } = within(canvasElement.parentElement!)

const createNotificationButton = await findByRole('button', { name: 'Create notification' })
await userEvent.click(createNotificationButton)
await userEvent.click(createNotificationButton)

const notifications = await findAllByText('There should only be one notification')
expect(notifications).toHaveLength(1)
Expand Down
Loading