Replace invalid className HTML attributes#3
Conversation
Signed-off-by: Harsh <harshmastic@gmail.com>
|
Closing this fork PR in favor of the upstream PR at processing#1324. |
There was a problem hiding this comment.
Pull request overview
Updates UI markup to avoid emitting invalid className attributes in rendered HTML by using class on intrinsic elements (Astro/Preact), while keeping component APIs (e.g., className props) intact.
Changes:
- Replace
className=...withclass=...on intrinsic elements across multiple Preact (.tsx/.jsx) components. - Rename destructured
classprop in an Astro component to avoid confusing local naming (className→sectionClass). - Remove debug logging and tidy formatting in copy-to-clipboard and code embed components.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/components/SearchResults/index.tsx | Convert intrinsic element styling from className to class; minor formatting cleanup. |
| src/components/RelatedItems/index.astro | Rename local variable for class prop (sectionClass) and apply via class={...}. |
| src/components/ReferenceDirectoryWithFilter/index.tsx | Use class for intrinsic elements (div, h3) in TSX. |
| src/components/PageHeader/HomePage.astro | Extract heroImages and add explicit typing in map callbacks. |
| src/components/Nav/MainNavLinks.tsx | Use class for anchor elements using CSS modules. |
| src/components/Icon/index.tsx | Apply props.className to SVGs via class attribute. |
| src/components/Dropdown/index.tsx | Use class for intrinsic elements and minor JSX formatting. |
| src/components/CopyCodeButton/index.tsx | Remove console logs, normalize quotes/formatting, simplify onClick handler. |
| src/components/CodeEmbed/index.jsx | Switch intrinsic elements to class, reformat some expressions/props. |
| src/components/CircleButton/index.tsx | Use class for intrinsic <a>/<button> elements; minor formatting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return ( | ||
| <div className="py-2xl md:py-3xl"> | ||
| <div class="py-2xl md:py-3xl"> | ||
| <div class="sticky top-0 bg-bg-color"> | ||
| <p className="mt-0 pb-xs pt-md">{results.length} results found for</p> | ||
| <p class="mt-0 pb-xs pt-md">{results.length} results found for</p> | ||
| {renderBigSearchForm()} | ||
| </div> | ||
| <div className="no-scrollbar w-full overflow-x-scroll"> | ||
| <div class="no-scrollbar w-full overflow-x-scroll"> |
There was a problem hiding this comment.
PR description reports running npm test/Vitest, but the pasted output shows Errors 2 errors at the end. Please either fix/handle the unhandled errors so npm test exits cleanly, or clarify in the PR description why these errors are pre-existing and whether CI is expected to be green despite them (e.g., known flake / separate follow-up issue).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on refactoring the codebase to use the class attribute instead of className across various components, likely to align with Preact's requirements. Additionally, it cleans up console logs in the CopyCodeButton component and improves code formatting. I have provided feedback suggesting the use of useEffect for managing the copy state timeout in CopyCodeButton to prevent potential memory leaks and state updates on unmounted components.
| @@ -1,5 +1,5 @@ | |||
| import { useState } from 'preact/hooks'; | |||
| import { useLiveRegion } from '../hooks/useLiveRegion'; | |||
| import { useState } from "preact/hooks"; | |||
There was a problem hiding this comment.
To make the component more robust when handling the timeout for resetting the copy state, useEffect should be used. Please import it.
| import { useState } from "preact/hooks"; | |
| import { useState, useEffect } from "preact/hooks"; |
| const copyTextToClipboard = async () => { | ||
| console.log('Copy button clicked'); | ||
| console.log('Text to copy:', textToCopy); | ||
|
|
||
| try { | ||
| console.log('Using Clipboard API'); | ||
| await navigator.clipboard.writeText(textToCopy); | ||
| console.log('Text copied successfully'); | ||
|
|
||
| announce(announceOnCopy); | ||
|
|
||
| setIsCopied(true); | ||
| setTimeout(() => { | ||
| setIsCopied(false); | ||
| console.log('Copy state reset'); | ||
| }, 2000); | ||
| } catch (err) { | ||
| console.error('Clipboard API copy failed:', err); | ||
| console.error("Clipboard API copy failed:", err); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Using setTimeout directly within an event handler can lead to issues if the component unmounts before the timeout completes. This can cause a state update on an unmounted component, which is a memory leak and will trigger a warning in development.
A more robust approach is to use the useEffect hook to handle the side effect of resetting the isCopied state. This ensures that the timeout is properly cleaned up if the component unmounts.
After importing useEffect, you can add this hook to your component:
useEffect(() => {
if (!isCopied) return;
const timer = setTimeout(() => {
setIsCopied(false);
}, 2000);
return () => clearTimeout(timer);
}, [isCopied]);And this function should only be responsible for setting isCopied to true.
const copyTextToClipboard = async () => {
try {
await navigator.clipboard.writeText(textToCopy);
announce(announceOnCopy);
setIsCopied(true);
} catch (err) {
console.error("Clipboard API copy failed:", err);
}
};
Summary
Testing
22:06:33 [astro-mermaid] Setting up Mermaid integration
22:06:33 [astro-mermaid] Existing rehype plugins:
22:06:33 [astro-mermaid] Skipping ELK support
22:06:33 [vite] Re-optimizing dependencies because lockfile has changed
22:06:39 [content] Syncing content
22:06:39 [content] Synced content
22:06:39 [types] Generated 5.27s
22:06:39 [check] Getting diagnostics for Astro files in /home/hxrshxz/Desktop/Projects/harsh/p5.js-website...
�[96msrc/components/DonorBoxWidget/index.astro�[0m:�[93m13�[0m:�[93m5�[0m - �[93mwarning�[0m�[90m ts(6385): �[0m'frameborder' is deprecated.
�[7m13�[0m frameborder="0"
�[7m �[0m �[93m ~~~~~~~~~~~�[0m
�[96msrc/components/DonorBoxWidget/index.astro�[0m:�[93m11�[0m:�[93m5�[0m - �[93mwarning�[0m�[90m ts(6385): �[0m'scrolling' is deprecated.
�[7m11�[0m scrolling="no"
�[7m �[0m �[93m ~~~~~~~~~�[0m
�[96msrc/components/PageHeader/HomePage.astro�[0m:�[93m67�[0m:�[93m50�[0m - �[93mwarning�[0m�[90m ts(6133): �[0m'event' is declared but its value is never read.
�[7m67�[0m document.addEventListener("DOMContentLoaded", (event) => {
�[7m �[0m �[93m ~~~~~�[0m
�[96msrc/components/ReferenceDirectoryWithFilter/index.tsx�[0m:�[93m218�[0m:�[93m32�[0m - �[93mwarning�[0m�[90m ts(6385): �[0m'TargetedKeyboardEvent' is deprecated.
�[7m218�[0m onKeyUp={(e: JSX.TargetedKeyboardEvent) => {
�[7m �[0m �[93m ~~~~~~~~~~~~~~~~~~~~~�[0m
�[96msrc/components/VimeoEmbed/index.astro�[0m:�[93m5�[0m:�[93m131�[0m - �[93mwarning�[0m�[90m ts(6385): �[0m'frameborder' is deprecated.
�[7m5�[0m <iframe src={
https://player.vimeo.com/video/${props.id}?h=9c6ecb5431&color=f1678e} title={props.title} width="720" height="405" frameborder="0" allow="autoplay; fullscreen; picture-in-picture" allowfullscreen></iframe>�[7m �[0m �[93m ~~~~~~~~~~~�[0m
�[96msrc/layouts/ExamplesLayout.astro�[0m:�[93m59�[0m:�[93m39�[0m - �[93mwarning�[0m�[90m ts(6133): �[0m'i' is declared but its value is never read.
�[7m59�[0m examplesByCategory.map((category, i) => (
�[7m �[0m �[93m ~�[0m
�[96msrc/layouts/ReferenceItemLayout.astro�[0m:�[93m171�[0m:�[93m59�[0m - �[93mwarning�[0m�[90m ts(6133): �[0m'i' is declared but its value is never read.
�[7m171�[0m {examples.map(({ src: exampleCode, classes }, i: number) => {
�[7m �[0m �[93m ~�[0m
�[96msrc/layouts/TutorialLayout.astro�[0m:�[93m8�[0m:�[93m1�[0m - �[93mwarning�[0m�[90m ts(6133): �[0m'CodeBlockWrapper' is declared but its value is never read.
�[7m8�[0m import CodeBlockWrapper from "@components/CodeBlockWrapper/index.astro";
�[7m �[0m �[93m~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~�[0m
Result (193 files):
22:07:00 [astro-mermaid] Setting up Mermaid integration
22:07:00 [astro-mermaid] Existing rehype plugins:
22:07:00 [astro-mermaid] Skipping ELK support
22:07:00 [astro-mermaid] Setting up Mermaid integration
22:07:00 [astro-mermaid] Existing rehype plugins:
22:07:00 [astro-mermaid] Skipping ELK support
22:07:00 [astro-mermaid] Setting up Mermaid integration
22:07:00 [astro-mermaid] Existing rehype plugins:
22:07:00 [astro-mermaid] Skipping ELK support
�[1m�[46m RUN �[49m�[22m �[36mv4.0.18 �[39m�[90m/home/hxrshxz/Desktop/Projects/harsh/p5.js-website�[39m
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/basic.test.ts �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 5�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/scripts/utils.test.ts �[2m(�[22m�[2m1 test�[22m�[2m)�[22m�[32m 4�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/scripts/build-search.test.ts �[2m(�[22m�[2m1 test�[22m�[2m)�[22m�[32m 7�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/i18n/utils.test.ts �[2m(�[22m�[2m14 tests�[22m�[2m)�[22m�[32m 8�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/components/CodeFrame.test.tsx �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 52�[2mms�[22m�[39m
�[90mstdout�[2m | test/scripts/build-contributor-docs.test.ts
�[22m�[39mBuilding contributor docs...
Considering cloning repo: https://github.com/processing/p5.js.git branch: v1.11.12 into path: /home/hxrshxz/Desktop/Projects/harsh/p5.js-website/in/p5.js/
�[90mstdout�[2m | test/scripts/build-contributor-docs.test.ts
�[22m�[39mRecent version of library repo already exists, skipping clone...
�[90mstdout�[2m | test/scripts/build-contributor-docs.test.ts
�[22m�[39mCleaning out current content collection...
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/scripts/build-contributor-docs.test.ts �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 3�[2mms�[22m�[39m
�[90mstdout�[2m | test/scripts/build-reference.test.ts
�[22m�[39mConsidering cloning repo: https://github.com/processing/p5.js.git branch: v1.11.12 into path: /home/hxrshxz/Desktop/Projects/harsh/p5.js-website/src/scripts/parsers/in/p5.js
�[90mstdout�[2m | test/scripts/build-reference.test.ts
�[22m�[39mRecent version of library repo already exists, skipping clone...
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/components/CodeEmbed.test.tsx �[2m(�[22m�[2m2 tests�[22m�[2m)�[22m�[32m 99�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[43m node �[49m�[39m test/pages/_utils.test.ts �[2m(�[22m�[2m7 tests�[22m�[2m)�[22m�[32m 5�[2mms�[22m�[39m
�[32m✓�[39m �[30m�[45m DOM �[49m�[39m test/scripts/build-reference.test.ts �[2m(�[22m�[2m5 tests�[22m�[2m)�[22m�[32m 4�[2mms�[22m�[39m
�[2m Test Files �[22m �[1m�[32m9 passed�[39m�[22m�[90m (9)�[39m
�[2m Tests �[22m �[1m�[32m36 passed�[39m�[22m�[90m (36)�[39m
�[2m Errors �[22m �[1m�[31m2 errors�[39m�[22m
�[2m Start at �[22m 22:07:11
�[2m Duration �[22m 23.64s�[2m (transform 4.54s, setup 0ms, import 7.38s, tests 187ms, environment 5.74s)�[22m (fails on clean too with existing unhandled errors in contributor docs/reference builder tests)