-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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(banner) hide #7058
Feat(banner) hide #7058
Changes from 4 commits
b3b0bca
d220af9
2018e3d
8457dc5
1a4b548
1b26606
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 |
---|---|---|
@@ -1,21 +1,53 @@ | ||
'use client'; | ||
|
||
import { useEffect, useState } from 'react'; | ||
import type { FC } from 'react'; | ||
|
||
import Banner from '@/components/Common/Banner'; | ||
import { useLocalStorage } from '@/hooks'; | ||
import { siteConfig } from '@/next.json.mjs'; | ||
import { dateIsBetween } from '@/util/dateUtils'; | ||
import { twoDateToUIID } from '@/util/stringUtils'; | ||
|
||
type BannerState = { | ||
uuid: string; | ||
hideBanner: boolean; | ||
}; | ||
|
||
const WithBanner: FC<{ section: string }> = ({ section }) => { | ||
const banner = siteConfig.websiteBanners[section]; | ||
const UUID = twoDateToUIID(banner.startDate, banner.endDate); | ||
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. nit: at first glance, I thought we were using the uuid package here due to the name. Not sure if it needs to change, but calling attention to the momentary confusion. I think the way you did this was clever overall. 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. We can call it 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. This feels so weird. |
||
const [shouldDisplay, setShouldDisplay] = useState(false); | ||
const [bannerState, setBannerState] = useLocalStorage<BannerState>('banner', { | ||
uuid: UUID, | ||
hideBanner: false, | ||
}); | ||
|
||
if (banner && dateIsBetween(banner.startDate, banner.endDate)) { | ||
useEffect(() => { | ||
if (dateIsBetween(banner.startDate, banner.endDate)) { | ||
setShouldDisplay(!bannerState.hideBanner); | ||
} | ||
if (bannerState.uuid !== UUID) { | ||
setBannerState({ uuid: UUID, hideBanner: false }); | ||
} | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [bannerState.hideBanner]); | ||
|
||
const handleBannerHiding = () => { | ||
setBannerState({ ...bannerState, hideBanner: true }); | ||
}; | ||
|
||
if (shouldDisplay) { | ||
return ( | ||
<Banner type={banner.type} link={banner.link}> | ||
<Banner | ||
type={banner.type} | ||
link={banner.link} | ||
onHiding={handleBannerHiding} | ||
> | ||
{banner.text} | ||
</Banner> | ||
); | ||
} | ||
|
||
return null; | ||
}; | ||
|
||
export default WithBanner; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { render } from '@testing-library/react'; | ||
import { act } from 'react-dom/test-utils'; | ||
|
||
import { useLocalStorage } from '@/hooks/react-client/useLocalStorage'; | ||
|
||
describe('useLocalStorage', () => { | ||
beforeEach(() => { | ||
Object.defineProperty(window, 'localStorage', { | ||
value: { | ||
getItem: jest.fn(), | ||
setItem: jest.fn(), | ||
}, | ||
writable: true, | ||
}); | ||
}); | ||
|
||
it('should initialize with the provided initial value', () => { | ||
render(() => { | ||
const [value] = useLocalStorage('testKey', 'initialValue'); | ||
expect(value).toBe('initialValue'); | ||
}); | ||
}); | ||
|
||
it('should update localStorage when value changes', () => { | ||
render(() => { | ||
const TestComponent = () => { | ||
const [value, setValue] = useLocalStorage('testKey', 'initialValue'); | ||
|
||
act(() => { | ||
setValue('newValue'); | ||
}); | ||
|
||
return <div>{value}</div>; | ||
}; | ||
|
||
const { container } = render(<TestComponent />); | ||
|
||
expect(window.localStorage.setItem).toHaveBeenCalledWith( | ||
'testKey', | ||
JSON.stringify('newValue') | ||
); | ||
expect(container.textContent).toBe('newValue'); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
'use client'; | ||
import { useEffect, useState } from 'react'; | ||
|
||
const useLocalStorage = <T>(key: string, initialValue?: T) => { | ||
canerakdas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const [value, setValue] = useState<T>(() => { | ||
if (typeof window !== 'undefined') { | ||
const item = window.localStorage.getItem(key); | ||
return item ? JSON.parse(item) : initialValue; | ||
} | ||
return initialValue; | ||
}); | ||
|
||
useEffect(() => { | ||
if (typeof window !== 'undefined') { | ||
window.localStorage.setItem(key, JSON.stringify(value)); | ||
} | ||
}, [key, value]); | ||
|
||
return [value, setValue] as const; | ||
}; | ||
|
||
export default useLocalStorage; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,3 +28,10 @@ export const dashToCamelCase = (str: string) => | |
// remove leftover - which don't match the above regex. Like 'es-2015' | ||
.replace(/-/g, '') | ||
.replace(/^[A-Z]/, chr => chr.toLowerCase()); | ||
|
||
export const twoDateToUIID = (date1: string, date2: string): string => { | ||
const date1String = date1.split('T')[0]; | ||
const date2String = date2.split('T')[0]; | ||
Comment on lines
+33
to
+34
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. 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. I'll have to reread the split docs because split automatically gives you a table that can be unique. 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. But we can also use |
||
|
||
return `${date1String}-${date2String}`; | ||
}; |
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.
I suggest a different name here.
onHiding
to me implies it's a handler, when it's really a conditional state or mode that the banner can opt-into.isHideable
would not the boolean nature, and nod to the story nameHideable