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

Feat(banner) hide #7058

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 26 additions & 7 deletions apps/site/components/Common/Banner/index.module.css
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
.banner {
@apply flex
w-full
flex-row
items-center
justify-center
gap-2
px-8
py-3
text-sm;
items-center;

.content {
@apply flex
w-full
flex-row
items-center
justify-center
gap-2
px-8
py-3
text-sm;
}

&,
a {
Expand All @@ -25,6 +31,19 @@
@apply size-4
text-white/50;
}

.close {
@apply pr-4
transition-transform;

&:hover {
@apply text-white;

svg {
@apply scale-110;
}
}
}
}

.default {
Expand Down
9 changes: 9 additions & 0 deletions apps/site/components/Common/Banner/index.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,13 @@ export const NoLink: Story = {
},
};

export const Hideable: Story = {
args: {
children: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit.',
type: 'default',
link: '/',
onHiding: () => {},
},
};

export default { component: Banner } as Meta;
34 changes: 27 additions & 7 deletions apps/site/components/Common/Banner/index.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { ArrowUpRightIcon } from '@heroicons/react/24/outline';
import { ArrowUpRightIcon, XMarkIcon } from '@heroicons/react/24/outline';
import classNames from 'classnames';
import { useTranslations } from 'next-intl';
import type { FC, PropsWithChildren } from 'react';

import Link from '@/components/Link';
Expand All @@ -8,17 +10,35 @@ import styles from './index.module.css';
type BannerProps = {
link?: string;
type?: 'default' | 'warning' | 'error';
onHiding?: () => void;
};

const Banner: FC<PropsWithChildren<BannerProps>> = ({
type = 'default',
link,
onHiding,
children,
Copy link
Collaborator

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 name Hideable

}) => (
<div className={`${styles.banner} ${styles[type] || styles.default}`}>
{link ? <Link href={link}>{children}</Link> : children}
{link && <ArrowUpRightIcon />}
</div>
);
}) => {
const t = useTranslations('components.common.banner');

return (
<div className={classNames(styles.banner, styles[type])}>
<span className={styles.content}>
{link ? <Link href={link}>{children}</Link> : children}
{link && <ArrowUpRightIcon />}
</span>
{onHiding && (
<button
aria-label={t('hide')}
className={styles.close}
title={t('hide')}
onClick={onHiding}
>
<XMarkIcon />
</button>
)}
</div>
);
};

export default Banner;
40 changes: 36 additions & 4 deletions apps/site/components/withBanner.tsx
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can call it generatekeyForBanner. But UIID mean Universally unique identifier.

Copy link
Member

Choose a reason for hiding this comment

The 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');
});
});
});
1 change: 1 addition & 0 deletions apps/site/hooks/react-client/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ export { default as useKeyboardCommands } from './useKeyboardCommands';
export { default as useClickOutside } from './useClickOutside';
export { default as useBottomScrollListener } from './useBottomScrollListener';
export { default as useNavigationState } from './useNavigationState';
export { default as useLocalStorage } from './useLocalStorage';
22 changes: 22 additions & 0 deletions apps/site/hooks/react-client/useLocalStorage.ts
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;
7 changes: 7 additions & 0 deletions apps/site/util/__tests__/stringUtils.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {
getAcronymFromString,
parseRichTextIntoPlainText,
dashToCamelCase,
twoDateToUIID,
} from '@/util/stringUtils';

describe('String utils', () => {
Expand Down Expand Up @@ -72,4 +73,10 @@ describe('String utils', () => {
it('dashToCamelCase returns correct camelCase with numbers', () => {
expect(dashToCamelCase('foo-123-bar')).toBe('foo123Bar');
});

it('twoDateToUIID returns the correct UUID', () => {
const date = '2024-01-01T00:00:00.000Z';
const date2 = '2024-01-01T00:00:00.000Z';
expect(twoDateToUIID(date, date2)).toBe('2024-01-01-2024-01-01');
});
});
7 changes: 7 additions & 0 deletions apps/site/util/stringUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

nothing seems to enforce this as a a proper date string over within site.json, which could lead to bugs or errors here splitting on T

image

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'll have to reread the split docs because split automatically gives you a table that can be unique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we can also use new Date and throw error if not right date


return `${date1String}-${date2String}`;
};
3 changes: 3 additions & 0 deletions packages/i18n/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@
"previous": "Previous"
},
"common": {
"banner": {
"hide": "Dismiss this banner"
},
"breadcrumbs": {
"navigateToHome": "Navigate to Home"
},
Expand Down
Loading