-
Notifications
You must be signed in to change notification settings - Fork 45
feat: create announcements banner #617
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
base: main
Are you sure you want to change the base?
Changes from all commits
022bfa9
c3d7a61
d45ba5a
f85c9fa
8a886b2
2b5603e
cc48c6a
a7434e4
bce9501
38e168b
36a4a21
149c1e1
44259a0
f426f4f
7e49669
cf2b7da
9b10132
ad9ae89
300d562
f1b7d56
85a3245
25c763c
298cbf5
e2a76a1
cc5383d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { ArrowUpRightIcon } from '@heroicons/react/24/outline'; | ||
| import Banner from '@node-core/ui-components/Common/Banner'; | ||
|
|
||
| import styles from './index.module.css'; | ||
|
|
||
| /** | ||
| * @param {import('./types.d.ts').AnnouncementBannerProps} props | ||
| */ | ||
| const AnnouncementBanner = ({ banners }) => ( | ||
| <div role="region" aria-label="Announcements" className={styles.banners}> | ||
| {banners.map(banner => ( | ||
| <Banner key={banner.text} type={banner.type}> | ||
araujogui marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| {banner.link ? ( | ||
| <a href={banner.link} target="_blank" rel="noopener noreferrer"> | ||
| {banner.text} | ||
| </a> | ||
| ) : ( | ||
| banner.text | ||
| )} | ||
| {banner.link && <ArrowUpRightIcon />} | ||
| </Banner> | ||
| ))} | ||
araujogui marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </div> | ||
| ); | ||
|
|
||
| export default AnnouncementBanner; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { describe, it } from 'node:test'; | ||
|
|
||
| import { loadBanners } from '../loadBanners.mjs'; | ||
|
|
||
| const PAST = new Date(Date.now() - 86_400_000).toISOString(); // yesterday | ||
| const FUTURE = new Date(Date.now() + 86_400_000).toISOString(); // tomorrow | ||
|
|
||
| const makeResponse = (banners, ok = true) => ({ | ||
| ok, | ||
| json: async () => ({ websiteBanners: banners }), | ||
| }); | ||
|
|
||
| describe('loadBanners', () => { | ||
| describe('fetch behavior', () => { | ||
| it('fetches from the given URL', async t => { | ||
| t.mock.method(global, 'fetch', () => Promise.resolve(makeResponse({}))); | ||
|
|
||
| await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.equal(global.fetch.mock.calls.length, 1); | ||
| assert.equal( | ||
| global.fetch.mock.calls[0].arguments[0], | ||
| 'https://example.com/site.json' | ||
| ); | ||
| }); | ||
|
|
||
| it('returns an empty array on non-ok response', async t => { | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve(makeResponse({}, false)) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, []); | ||
| }); | ||
|
|
||
| it('handles fetch errors silently', async t => { | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.reject(new Error('Network error')) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, []); | ||
| }); | ||
|
|
||
| it('returns an empty array when remoteConfig is absent', async t => { | ||
| t.mock.method(global, 'fetch', () => Promise.resolve(makeResponse({}))); | ||
|
|
||
| const result = await loadBanners(undefined, null); | ||
|
|
||
| assert.deepEqual(result, []); | ||
| assert.equal(global.fetch.mock.calls.length, 0); | ||
| }); | ||
| }); | ||
|
|
||
| describe('banner selection', () => { | ||
| it('returns the active global (index) banner', async t => { | ||
| const banner = { text: 'Global banner', type: 'warning' }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve(makeResponse({ index: banner })) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, [banner]); | ||
| }); | ||
|
|
||
| it('returns the active version-specific banner', async t => { | ||
| const banner = { text: 'v20 banner', type: 'warning' }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve(makeResponse({ v20: banner })) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', 20); | ||
|
|
||
| assert.deepEqual(result, [banner]); | ||
| }); | ||
|
|
||
| it('returns both global and version banners when both are active', async t => { | ||
| const globalBanner = { text: 'Global banner', type: 'warning' }; | ||
| const versionBanner = { text: 'v20 banner', type: 'error' }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve( | ||
| makeResponse({ index: globalBanner, v20: versionBanner }) | ||
| ) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', 20); | ||
|
|
||
| assert.deepEqual(result, [globalBanner, versionBanner]); | ||
| }); | ||
|
|
||
| it('returns global banner first, version banner second', async t => { | ||
| const globalBanner = { text: 'Global', type: 'warning' }; | ||
| const versionBanner = { text: 'v22', type: 'error' }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve( | ||
| makeResponse({ index: globalBanner, v22: versionBanner }) | ||
| ) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', 22); | ||
|
|
||
| assert.equal(result[0], globalBanner); | ||
| assert.equal(result[1], versionBanner); | ||
| }); | ||
|
|
||
| it('does not include the version banner when versionMajor is null', async t => { | ||
| const globalBanner = { text: 'Global banner', type: 'warning' }; | ||
| const versionBanner = { text: 'v20 banner', type: 'error' }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve( | ||
| makeResponse({ index: globalBanner, v20: versionBanner }) | ||
| ) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, [globalBanner]); | ||
| }); | ||
|
|
||
| it('returns an empty array when websiteBanners is absent', async t => { | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve({ ok: true, json: async () => ({}) }) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, []); | ||
| }); | ||
| }); | ||
|
|
||
| describe('date filtering', () => { | ||
| it('excludes a banner whose endDate has passed', async t => { | ||
| const banner = { text: 'Expired', type: 'warning', endDate: PAST }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve(makeResponse({ index: banner })) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, []); | ||
| }); | ||
|
|
||
| it('excludes a banner whose startDate is in the future', async t => { | ||
| const banner = { text: 'Upcoming', type: 'warning', startDate: FUTURE }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve(makeResponse({ index: banner })) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, []); | ||
| }); | ||
|
|
||
| it('includes a banner within its active date range', async t => { | ||
| const banner = { | ||
| text: 'Active', | ||
| type: 'warning', | ||
| startDate: PAST, | ||
| endDate: FUTURE, | ||
| }; | ||
| t.mock.method(global, 'fetch', () => | ||
| Promise.resolve(makeResponse({ index: banner })) | ||
| ); | ||
|
|
||
| const result = await loadBanners('https://example.com/site.json', null); | ||
|
|
||
| assert.deepEqual(result, [banner]); | ||
| }); | ||
| }); | ||
| }); |
|
Member
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. The usage of SERVER?: is worrysome, can you add a TODO comment so that we address this in the future? I believe that @avivkeller aims to solve for that soo ™️ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import { lazy, Suspense } from 'preact/compat'; | ||
|
|
||
| import AnnouncementBanner from './AnnouncementBanner.jsx'; | ||
| import { loadBanners } from './loadBanners.mjs'; | ||
|
|
||
| import { remoteConfig, versionMajor } from '#theme/config'; | ||
|
|
||
| // TODO: Revisit SERVER global usage. | ||
| const LazyBanners = SERVER | ||
| ? null | ||
| : lazy(async () => { | ||
| const active = await loadBanners(remoteConfig, versionMajor); | ||
|
|
||
| if (!active.length) { | ||
| return { default: () => null }; | ||
| } | ||
|
|
||
| return { default: () => <AnnouncementBanner banners={active} /> }; | ||
| }); | ||
|
|
||
| const RemoteLoadableBanner = SERVER | ||
| ? () => <div /> | ||
| : () => ( | ||
| <div> | ||
| <Suspense fallback={null}> | ||
| <LazyBanners /> | ||
| </Suspense> | ||
| </div> | ||
| ); | ||
|
|
||
| export default RemoteLoadableBanner; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| .banners { | ||
| animation: slideDown 300ms ease-out 400ms backwards; | ||
| } | ||
|
|
||
| @keyframes slideDown { | ||
| from { | ||
| opacity: 0; | ||
| transform: translateY(-0.5rem); | ||
| } | ||
| to { | ||
| opacity: 1; | ||
| transform: translateY(0); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import { isBannerActive } from '../../utils/banner.mjs'; | ||
|
|
||
| /** | ||
| * Fetches and returns active banners for the given version. | ||
| * Returns an empty array when remoteConfig is absent, the response is not ok, | ||
| * or on any fetch/parse failure. | ||
| * | ||
| * @param {string | undefined} remoteConfig | ||
| * @param {number | null} versionMajor | ||
| * @returns {Promise<Array<import('./types.d.ts').BannerEntry>>} | ||
| */ | ||
| export const loadBanners = async (remoteConfig, versionMajor) => { | ||
| if (!remoteConfig) { | ||
| return []; | ||
| } | ||
|
|
||
| try { | ||
| const res = await fetch(remoteConfig); | ||
|
|
||
araujogui marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| /** @type {import('./types.d.ts').RemoteConfig} */ | ||
| const { websiteBanners = {} } = await res.json(); | ||
araujogui marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const keys = ['index', versionMajor != null && `v${versionMajor}`].filter( | ||
| Boolean | ||
| ); | ||
|
|
||
| return keys | ||
| .map(key => websiteBanners[key]) | ||
| .filter(banner => banner && isBannerActive(banner)); | ||
| } catch { | ||
| return []; | ||
| } | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| import type { BannerProps } from '@node-core/ui-components/Common/Banner'; | ||
|
|
||
| export type BannerEntry = { | ||
| startDate?: string; | ||
| endDate?: string; | ||
| text: string; | ||
| link?: string; | ||
| type?: BannerProps['type']; | ||
| }; | ||
|
|
||
| export type RemoteConfig = { | ||
| websiteBanners?: Record<string, BannerEntry | undefined>; | ||
| }; | ||
|
|
||
| export type AnnouncementBannerProps = { | ||
| banners: BannerEntry[]; | ||
| }; | ||
|
|
||
| export type RemoteLoadableBannerProps = { | ||
| remoteConfig: string; | ||
| versionMajor: number | null; | ||
| }; | ||
|
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. Unused
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ declare module '#theme/config' { | |
|
|
||
| // From config generation | ||
| export const version: string; | ||
| export const versionMajor: number; | ||
|
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. New config module exports lack JSDoc documentationLow Severity The newly added Additional Locations (1)Triggered by learned rule: New config/type properties must have JSDoc documentation Reviewed by Cursor Bugbot for commit cc5383d. Configure here. |
||
| export const versions: Array<{ | ||
| url: string; | ||
| label: string; | ||
|
|
@@ -32,6 +33,7 @@ declare module '#theme/config' { | |
| export const editURL: string; | ||
| export const pages: Array<[string, string]>; | ||
| export const languageDisplayNameMap: Map<string[], string>; | ||
| export const remoteConfig: string; | ||
| } | ||
|
|
||
| // Omit Primitives from Metadata | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.