Skip to content
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
3 changes: 3 additions & 0 deletions cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
"creativecommons",
"cssmodules",
"customise",
"CWBI",
"deamdify",
"debugids",
"dedupDepth",
Expand Down Expand Up @@ -192,6 +193,8 @@
"spdy",
"sponsorkit",
"ssgrun",
"statlink",
"statview",
"styl",
"subchunks",
"Subdirs",
Expand Down
1 change: 1 addition & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export default defineConfig([
extends: [configs["browser/recommended-outdated-module"]],
files: [
"src/components/**/*.{js,jsx}",
"src/components/CarbonAd/useCarbonAd.mjs",
"src/utilities/test-local-storage.js",
"src/*.jsx",
"src/sw.js",
Expand Down
40 changes: 40 additions & 0 deletions src/components/CarbonAd/SideBar.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import useCarbonAd, { adColors } from "./useCarbonAd.mjs";
import "./carbon-ad.css";

/**
* Carbon ad card at the bottom of the docs sidebar. The sidebar is hidden
* below the `md` breakpoint, and the ad is neither shown nor requested there.
*/
export default function SideBarAd() {
const { ad, ref } = useCarbonAd("CWBI6K77", "(min-width: 768px)");

if (!ad) {
return null;
}

return (
<aside
ref={ref}
className="carbon-side"
style={adColors(ad)}
aria-label="Advertisement"
>
<a
href={ad.statlink}
target="_blank"
rel="sponsored noopener noreferrer"
className="carbon-side-link"
>
<img src={ad.largeImage} alt={ad.company} className="carbon-image" />
<span className="carbon-side-body">
<strong className="carbon-title">{ad.title}</strong>
<span>{ad.description}</span>
<span className="carbon-side-cta">{ad.callToAction}</span>
</span>
</a>
<a href={ad.ad_via_link} className="carbon-badge carbon-overlay">
Ad
</a>
</aside>
);
}
79 changes: 79 additions & 0 deletions src/components/CarbonAd/TopBar.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import PropTypes from "prop-types";
import { useState } from "react";
import CloseIcon from "../../styles/icons/cross.svg";
import useCarbonAd, { adColors } from "./useCarbonAd.mjs";
import "./carbon-ad.css";

const DISMISSED_KEY = "top-bar-ad-dismissed";

/**
* Carbon banner above the page content. Closing it hides the banner for the
* rest of the visit and does not request another ad.
*/
function Ad({ onClose }) {
const { ad, ref } = useCarbonAd("CWBI623E");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor · Functional Correctness · Quick win

Restrict the top-bar ad to md and wider.

useCarbonAd("CWBI623E") has no media argument. Phone-width visitors therefore request and see the banner. The PR states that ads are requested only at md and above. Pass "(min-width: 768px)" as the second argument.

This also removes the phone-width request and impression. A viewport that narrows after the fetch still shows the banner. Hide it with a CSS max-md:hidden class if that matters.

Proposed fix

--- "a/src/components/CarbonAd/TopBar.jsx"
+++ "b/src/components/CarbonAd/TopBar.jsx"
@@ -11,7 +11,7 @@
  * rest of the visit and does not request another ad.
  */
 function Ad({ onClose }) {
-  const { ad, ref } = useCarbonAd("CWBI623E");
+  const { ad, ref } = useCarbonAd("CWBI623E", "(min-width: 768px)");
 
   if (!ad) {
     return null;
Committable suggestion

IMPORTANT
Review the replacement and indentation, then test before committing.

Suggested change
const { ad, ref } = useCarbonAd("CWBI623E");
const { ad, ref } = useCarbonAd("CWBI623E", "(min-width: 768px)");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Move the argument (ad key) to a higher level variable


if (!ad) {
return null;
}

return (
<aside
ref={ref}
className="carbon-top"
style={adColors(ad)}
aria-label="Advertisement"
>
<a
href={ad.statlink}
target="_blank"
rel="sponsored noopener noreferrer"
className="carbon-top-link"
>
<img src={ad.image} alt={ad.company} className="carbon-logo" />
<span>{ad.description}</span>
<span className="carbon-cta">{ad.callToAction}</span>
</a>
<a href={ad.ad_via_link} className="carbon-badge">
Ad
</a>
<button
type="button"
onClick={onClose}
className="carbon-close"
aria-label="Close advertisement"
>
<CloseIcon
width={14}
height={14}
aria-hidden="true"
className="fill-current"
/>
</button>
</aside>
);
}

Ad.propTypes = {
onClose: PropTypes.func.isRequired,
};

Comment on lines +51 to +60

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor · Functional Correctness · Quick win

Supported by static analysis

Script executed:

sed -n '1,110p' src/components/CarbonAd/TopBar.jsx
sed -n '1,115p' src/components/CarbonAd/carbon-ad.css
sed -n '110,140p' src/components/Page/Page.jsx

Repository: webpack/webpack.js.org

Length of output: 4880


Script executed:

git diff --stat 9cfb83cff4c755585ef649a81d3c1a56e12b68fe 07a764ff37ca3fb891232a40000fc79086f4486a -- src/components/CarbonAd src/components/Page
cat -n src/components/CarbonAd/useCarbonAd.mjs
cat -n src/components/CarbonAd/TopBar.jsx
cat -n src/components/CarbonAd/Sidebar.jsx 2>/dev/null
cat -n src/components/Page/Page.jsx | sed -n '1,150p'
rg -n -F -e 'TopBarAd' -e 'SidebarAd' -e 'useCarbonAd(' -e '--breakpoint-md' -- src || test "$?" -eq 1
ls src/components/CarbonAd

Repository: webpack/webpack.js.org

Length of output: 12402


Script executed:

cat -n src/components/CarbonAd/SideBar.jsx
cat -n src/components/CarbonAd/carbon-ad.css
cat -n src/styles/tailwind.css | sed -n '1,40p'
rg -n -F -e 'from "../Page/Page' -e 'from "./Page/Page' -e '<Page' -e 'CarbonAd/SideBar' -- src || test "$?" -eq 1
git diff 9cfb83cff4c755585ef649a81d3c1a56e12b68fe 07a764ff37ca3fb891232a40000fc79086f4486a -- src/components/Page/Page.jsx

Repository: webpack/webpack.js.org

Length of output: 6936


Script executed:

cat -n src/components/Site/Site.jsx | sed -n '265,390p'
cat -n src/components/Sidebar/Sidebar.jsx | sed -n '1,120p'
rg -n -F -e 'PageElement' -e 'printable' -- src/components/Site/Site.jsx

Repository: webpack/webpack.js.org

Length of output: 8877


Restrict the top-bar ad to md and wider

On a phone-width documentation page, useCarbonAd("CWBI623E") can request and display the banner because it has no viewport guard. Gate the request below 768px and hide the banner when the viewport narrows.

export default function TopBarAd() {
// There is no storage while rendering on the server.
const [dismissed, setDismissed] = useState(
() => globalThis.sessionStorage?.getItem(DISMISSED_KEY) === "true",
);
Comment on lines +63 to +65

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor · Stability & Availability · Quick win

Guard the sessionStorage read.

Accessing globalThis.sessionStorage can throw a SecurityError when the browser blocks storage. The throw happens during render and can break the whole Page. Wrap the read in try/catch and default to false.

Proposed fix

--- "a/src/components/CarbonAd/TopBar.jsx"
+++ "b/src/components/CarbonAd/TopBar.jsx"
@@ -60,9 +60,13 @@
 
 export default function TopBarAd() {
   // There is no storage while rendering on the server.
-  const [dismissed, setDismissed] = useState(
-    () => globalThis.sessionStorage?.getItem(DISMISSED_KEY) === "true",
-  );
+  const [dismissed, setDismissed] = useState(() => {
+    try {
+      return globalThis.sessionStorage?.getItem(DISMISSED_KEY) === "true";
+    } catch {
+      return false;
+    }
+  });
 
   if (dismissed) {
     return null;

Based on learnings: flag storage calls only when nothing handles thrown exceptions.

Committable suggestion

IMPORTANT
Review the replacement and indentation, then test before committing.

Suggested change
const [dismissed, setDismissed] = useState(
() => globalThis.sessionStorage?.getItem(DISMISSED_KEY) === "true",
);
const [dismissed, setDismissed] = useState(() => {
try {
return globalThis.sessionStorage?.getItem(DISMISSED_KEY) === "true";
} catch {
return false;
}
});

Source: Learnings


if (dismissed) {
return null;
}

return (
<Ad
onClose={() => {
sessionStorage.setItem(DISMISSED_KEY, "true");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor · Stability & Availability · Quick win

Handle a storage failure in the close handler.

sessionStorage.setItem throws when storage is blocked or full (for example, SecurityError or QuotaExceededError). The throw happens inside the click handler, so setDismissed(true) never runs. The banner then cannot be closed. The read at Line 64 is unguarded as well. Reading sessionStorage can throw when storage access is blocked.

Wrap both accesses in try/catch. Always call setDismissed(true).

Proposed fix

--- "a/src/components/CarbonAd/TopBar.jsx"
+++ "b/src/components/CarbonAd/TopBar.jsx"
@@ -70,10 +70,14 @@
 
   return (
     <Ad
       onClose={() => {
-        sessionStorage.setItem(DISMISSED_KEY, "true");
+        try {
+          sessionStorage.setItem(DISMISSED_KEY, "true");
+        } catch {
+          // Storage is unavailable. Dismiss for this mount only.
+        }
         setDismissed(true);
       }}
     />
   );
 }

Based on learnings: flag storage calls only when nothing handles thrown exceptions.

Committable suggestion

IMPORTANT
Review the replacement and indentation, then test before committing.

Suggested change
sessionStorage.setItem(DISMISSED_KEY, "true");
try {
sessionStorage.setItem(DISMISSED_KEY, "true");
} catch {
// Storage is unavailable. Dismiss for this mount only.
}

Source: Learnings

setDismissed(true);
}}
/>
);
}
106 changes: 106 additions & 0 deletions src/components/CarbonAd/carbon-ad.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
@reference "../../styles/tailwind.css";

/* Colors, copy and images belong to the approved creative. Each ad sets its
palette through the custom properties from `adColors`. These names must stay
global: CSS modules would rename them and they would no longer match. */
.carbon-top,
.carbon-side {
@apply relative
bg-(--ad-bg)
text-(color:--ad-text)
print:hidden
dark:bg-(--ad-bg-dark)
dark:text-(color:--ad-text-dark);

& a {
@apply text-inherit no-underline;
}
}

.carbon-badge {
@apply shrink-0
rounded-sm
border
border-current
px-1.5
text-xs
leading-4
opacity-70
hover:opacity-100;
}

.carbon-top {
@apply mb-4 flex items-center gap-3 rounded-md px-4 py-2 text-sm lg:min-h-14;
}

.carbon-top-link {
@apply flex min-w-0 flex-1 items-center gap-3;
}

.carbon-logo {
@apply size-8 shrink-0 rounded-sm;
}

.carbon-cta {
@apply hidden
shrink-0
rounded-md
bg-(--ad-cta-bg)
px-3
py-1
text-xs
font-semibold
whitespace-nowrap
text-(color:--ad-cta-text)
sm:block
dark:bg-(--ad-cta-bg-dark)
dark:text-(color:--ad-cta-text-dark);
}

.carbon-close {
@apply flex
shrink-0
cursor-pointer
items-center
rounded-full
border-none
bg-transparent
p-0.5
text-inherit
opacity-60
transition-opacity
hover:opacity-100;
}

.carbon-side {
@apply mt-6 w-full overflow-hidden rounded-lg;
}

.carbon-side-link {
@apply flex flex-col;
}

.carbon-image {
@apply aspect-square w-full object-cover;
}

.carbon-side-body {
@apply flex flex-col items-start gap-1 p-3.5 text-xs;
}

.carbon-title {
@apply font-semibold;
}

.carbon-side-cta {
@apply mt-2 rounded-full border border-current px-3 py-1 font-semibold;
}

.carbon-overlay {
@apply absolute
top-2
right-2
border-transparent
bg-neutral-950/50
text-white;
}
92 changes: 92 additions & 0 deletions src/components/CarbonAd/useCarbonAd.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { useEffect, useRef, useState } from "react";

/**
* Advertiser colors are part of the approved creative and are applied as
* custom properties. Dark values fall back to the light ones.
*
* @param {Record<string, string>} ad
* @returns {import("react").CSSProperties}
*/
export function adColors(ad) {
return {
"--ad-bg": ad.backgroundColor,
"--ad-bg-dark": ad.backgroundColorDark ?? ad.backgroundColor,
"--ad-text": ad.textColor,
"--ad-text-dark": ad.textColorDark ?? ad.textColor,
"--ad-cta-bg": ad.ctaBackgroundColor,
"--ad-cta-bg-dark": ad.ctaBackgroundColorDark ?? ad.ctaBackgroundColor,
"--ad-cta-text": ad.ctaTextColor,
"--ad-cta-text-dark": ad.ctaTextColorDark ?? ad.ctaTextColor,
};
}

/**
* Request one Carbon ad and send its view beacon once at least half of the
* ad has been visible for one continuous second.
*
* A `media` query that does not match skips the request entirely. The hook
* fetches once per mount, so callers remount it when the page changes.
*
* @param {string} zone
* @param {string} [media]
*/
export default function useCarbonAd(zone, media) {
const [ad, setAd] = useState(null);
const ref = useRef(null);

useEffect(() => {
if (media && globalThis.matchMedia?.(media).matches !== true) {
return;
}

if (typeof globalThis.fetch !== "function") {
return;
}

const controller = new AbortController();

globalThis
.fetch(`https://srv.carbonads.net/ads/${zone}.json`, {
signal: controller.signal,
})
.then((response) => (response.ok ? response.json() : null))
.then((data) => {
if (data?.ads?.[0]?.statlink) {
setAd(data.ads[0]);
}
})
.catch(() => {});

return () => controller.abort();
}, [zone, media]);

useEffect(() => {
if (!ad?.statview || !ref.current) {
return;
}

let timer;
const observer = new IntersectionObserver(
(entries) => {
clearTimeout(timer);

if (entries.at(-1).intersectionRatio >= 0.5) {
timer = setTimeout(() => {
new Image().src = ad.statview;
observer.disconnect();
}, 1000);
}
},
{ threshold: 0.5 },
);

observer.observe(ref.current);

return () => {
clearTimeout(timer);
observer.disconnect();
};
}, [ad]);

return { ad, ref };
}
2 changes: 2 additions & 0 deletions src/components/Page/Page.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useEffect, useState } from "react";
import { useLocation } from "react-router";

// Import Components
import TopBarAd from "../CarbonAd/TopBar.jsx";
import Contributors from "../Contributors/Contributors.jsx";
import Link from "../Link/Link.jsx";
import Markdown from "../Markdown/Markdown.jsx";
Expand Down Expand Up @@ -120,6 +121,7 @@ export default function Page(props) {
id="main-content"
className="flex-auto relative overflow-x-hidden py-[1.5em] px-[1em] md:flex-[3] md:p-[1.5em]"
>
{pathname.includes("/printable") ? null : <TopBarAd key={pathname} />}
<Markdown>
<h1>{title}</h1>
{rest.date && pathname.startsWith("/blog/") && !isBlogIndex && (
Expand Down
4 changes: 4 additions & 0 deletions src/components/Sidebar/Sidebar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import PropTypes from "prop-types";
import { useEffect, useState } from "react";
import DownIcon from "../../styles/icons/chevron-down.svg";
import LoadingIcon from "../../styles/icons/loading.svg";
import SideBarAd from "../CarbonAd/SideBar.jsx";
import Print from "../Print/Print.jsx";
import SidebarItem from "../SidebarItem/SidebarItem.jsx";

Expand Down Expand Up @@ -89,6 +90,9 @@ export default function Sidebar({ className = "", pages, currentPage }) {
</div>
);
})}
{currentPage?.includes("/printable") ? null : (
<SideBarAd key={currentPage} />
)}
</div>
</nav>
);
Expand Down
Loading