-
Notifications
You must be signed in to change notification settings - Fork 27
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
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 |
|---|---|---|
|
|
@@ -34,6 +34,8 @@ export default { | |
| imports: { | ||
| '#config/Logo': '@node-core/ui-components/Common/NodejsLogo', | ||
| }, | ||
| remoteConfig: | ||
| 'https://gist.githubusercontent.com/araujogui/8ea72ffaf574f58fca1482e764e8b5c8/raw/16af51e4efbf37da7b6aff9b7e5dd967d955aacf/api-docs.config.json', | ||
|
Comment on lines
+37
to
+38
|
||
| }, | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ export type Generator = GeneratorMetadata< | |||||
| templatePath: string; | ||||||
| title: string; | ||||||
| imports: Record<string, string>; | ||||||
| remoteConfig: string | null; | ||||||
|
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.
Suggested change
|
||||||
| }, | ||||||
| Generate<Array<JSXContent>, AsyncGenerator<{ html: string; css: string }>> | ||||||
| >; | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||||||||||||||||||||||||||||||
| import { ArrowUpRightIcon } from '@heroicons/react/24/outline'; | ||||||||||||||||||||||||||||||||||
| import Banner from '@node-core/ui-components/Common/Banner'; | ||||||||||||||||||||||||||||||||||
| import { useEffect, useState } from 'preact/hooks'; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import { STATIC_DATA } from '../../constants.mjs'; | ||||||||||||||||||||||||||||||||||
| import { isBannerActive } from '../../utils/banner.mjs'; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** @import { BannerEntry, RemoteConfig } from './types.d.ts' */ | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Asynchronously fetches and displays announcement banners from the remote config. | ||||||||||||||||||||||||||||||||||
| * Global banners are rendered above version-specific ones. | ||||||||||||||||||||||||||||||||||
| * Non-blocking: silently ignores fetch/parse failures. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| export default () => { | ||||||||||||||||||||||||||||||||||
| const [banners, setBanners] = useState(/** @type {BannerEntry[]} */ ([])); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||||||||||||||||
| const { remoteConfig, versionMajor } = STATIC_DATA; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if (!remoteConfig) { | ||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| fetch(remoteConfig, { | ||||||||||||||||||||||||||||||||||
| signal: AbortSignal.timeout(2500), | ||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||
| .then(async res => { | ||||||||||||||||||||||||||||||||||
| if (!res.ok) { | ||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** @type {RemoteConfig} */ | ||||||||||||||||||||||||||||||||||
| const config = await res.json(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const active = []; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const globalBanner = config.global?.banner; | ||||||||||||||||||||||||||||||||||
| if (globalBanner && isBannerActive(globalBanner)) { | ||||||||||||||||||||||||||||||||||
| active.push(globalBanner); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const versionBanner = config[`v${versionMajor}`]?.banner; | ||||||||||||||||||||||||||||||||||
| if (versionBanner && isBannerActive(versionBanner)) { | ||||||||||||||||||||||||||||||||||
| active.push(versionBanner); | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+45
|
||||||||||||||||||||||||||||||||||
| const versionBanner = config[`v${versionMajor}`]?.banner; | |
| if (versionBanner && isBannerActive(versionBanner)) { | |
| active.push(versionBanner); | |
| if (versionMajor != null) { | |
| const versionBanner = config[`v${versionMajor}`]?.banner; | |
| if (versionBanner && isBannerActive(versionBanner)) { | |
| active.push(versionBanner); | |
| } |
avivkeller marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 20, 2026
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.
The key used for banners without links will be undefined, which can cause React reconciliation issues. When there are multiple banners without links, they will all have the same key (undefined), leading to incorrect updates and potential bugs in the banner display. Consider using the banner text or index as a fallback key, or use a combination of fields to ensure uniqueness. For example: key={banner.link || banner.text} or key={banner.link || ${banner.text}-${index}}.
| {banners.map(banner => ( | |
| <Banner key={banner.link} type={banner.type}> | |
| {banners.map((banner, index) => ( | |
| <Banner key={banner.link || `${banner.text}-${index}`} type={banner.type}> |
Copilot
AI
Feb 20, 2026
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.
External URLs from the remote config are used directly in href attributes without validation. While React/Preact provides some protection against javascript: protocol URLs, there's still a potential security risk if the remote config source is compromised. Consider validating that banner.link uses an approved protocol (https://) or implementing an allowlist of acceptable domains to ensure only trusted URLs are rendered. This is especially important since the config is fetched from an external source.
Copilot
AI
Feb 20, 2026
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.
The ArrowUpRightIcon typically indicates an external link that opens in a new tab, but the anchor tag is missing target="_blank" and rel="noopener noreferrer" attributes. If the intention is for these links to open in a new tab (as the icon suggests), add target="_blank". For security and accessibility, also include rel="noopener noreferrer" to prevent the linked page from accessing window.opener, and consider adding an aria-label to indicate the link opens externally.
| {banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text} | |
| {banner.link && <ArrowUpRightIcon />} | |
| {banner.link ? ( | |
| <a | |
| href={banner.link} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| aria-label={`${banner.text} (opens in a new tab)`} | |
| > | |
| {banner.text} | |
| </a> | |
| ) : ( | |
| banner.text | |
| )} | |
| {banner.link && <ArrowUpRightIcon aria-hidden="true" />} |
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.
nit: this can go within the above line
Copilot
AI
Feb 20, 2026
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.
The wrapper div element lacks semantic meaning. Consider using a more semantic HTML element like <aside> or adding appropriate ARIA attributes to indicate this is an announcements region. For example, <div role="region" aria-label="Announcements"> would improve accessibility by clearly identifying the purpose of this section to screen readers.
| <div> | |
| {banners.map(banner => ( | |
| <Banner key={banner.link} type={banner.type}> | |
| {banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text} | |
| {banner.link && <ArrowUpRightIcon />} | |
| </Banner> | |
| ))} | |
| </div> | |
| <aside aria-label="Announcements"> | |
| {banners.map(banner => ( | |
| <Banner key={banner.link} type={banner.type}> | |
| {banner.link ? <a href={banner.link}>{banner.text}</a> : banner.text} | |
| {banner.link && <ArrowUpRightIcon />} | |
| </Banner> | |
| ))} | |
| </aside> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| 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 = Record<string, { banner?: BannerEntry } | undefined>; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import assert from 'node:assert/strict'; | ||
| import { describe, it } from 'node:test'; | ||
|
|
||
| import { isBannerActive } from '../banner.mjs'; | ||
|
|
||
| const PAST = new Date(Date.now() - 86_400_000).toISOString(); // yesterday | ||
| const FUTURE = new Date(Date.now() + 86_400_000).toISOString(); // tomorrow | ||
|
|
||
| const banner = (overrides = {}) => ({ | ||
| text: 'Test banner', | ||
| ...overrides, | ||
| }); | ||
|
|
||
| describe('isBannerActive', () => { | ||
| describe('no startDate, no endDate', () => { | ||
| it('is always active', () => { | ||
| assert.equal(isBannerActive(banner()), true); | ||
| }); | ||
| }); | ||
|
|
||
| describe('startDate only', () => { | ||
| it('is active when startDate is in the past', () => { | ||
| assert.equal(isBannerActive(banner({ startDate: PAST })), true); | ||
| }); | ||
|
|
||
| it('is not active when startDate is in the future', () => { | ||
| assert.equal(isBannerActive(banner({ startDate: FUTURE })), false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('endDate only', () => { | ||
| it('is active when endDate is in the future', () => { | ||
| assert.equal(isBannerActive(banner({ endDate: FUTURE })), true); | ||
| }); | ||
|
|
||
| it('is not active when endDate is in the past', () => { | ||
| assert.equal(isBannerActive(banner({ endDate: PAST })), false); | ||
| }); | ||
| }); | ||
|
|
||
| describe('startDate and endDate', () => { | ||
| it('is active when now is within the range', () => { | ||
| assert.equal( | ||
| isBannerActive(banner({ startDate: PAST, endDate: FUTURE })), | ||
| true | ||
| ); | ||
| }); | ||
|
|
||
| it('is not active when now is before the range', () => { | ||
| assert.equal( | ||
| isBannerActive(banner({ startDate: FUTURE, endDate: FUTURE })), | ||
| false | ||
| ); | ||
| }); | ||
|
|
||
| it('is not active when now is after the range', () => { | ||
| assert.equal( | ||
| isBannerActive(banner({ startDate: PAST, endDate: PAST })), | ||
| false | ||
| ); | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| /** @import { BannerEntry } from '../components/AnnouncementBanner/types' */ | ||
|
|
||
| /** | ||
| * Checks whether a banner should be displayed based on its date range. | ||
| * Both `startDate` and `endDate` are optional; if omitted the banner is | ||
| * considered open-ended in that direction. | ||
| * | ||
| * @param {BannerEntry} banner | ||
| * @returns {boolean} | ||
| */ | ||
| export const isBannerActive = banner => { | ||
| const now = Date.now(); | ||
| if (banner.startDate && now < new Date(banner.startDate).getTime()) { | ||
| return false; | ||
| } | ||
| if (banner.endDate && now > new Date(banner.endDate).getTime()) { | ||
| return false; | ||
| } | ||
| return true; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,8 @@ export const createStaticData = () => { | |
| shikiDisplayNameMap, | ||
| title: config.title, | ||
| repository: config.repository, | ||
| versionMajor: config.version?.major ?? null, | ||
| remoteConfig: config.remoteConfig ?? null, | ||
|
Comment on lines
+35
to
+36
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 idea of this data is for components that can't accept the data as properties. Can we instead pass these values as properties to the component, which produces a smaller bundle? |
||
| }; | ||
| }; | ||
|
|
||
|
|
||
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.
The hardcoded Gist URL includes a specific commit hash in the raw URL. This means the configuration is pinned to a specific version and won't automatically update when the Gist is modified. If the intent is to fetch the latest version dynamically, the URL should use the format without the commit hash (e.g., ending with
/raw/api-docs.config.json). If the commit hash is intentional for stability, consider documenting why this specific version is pinned and how it should be updated.