Feat: Generate-Registry Script#28
Conversation
|
@luth-v is attempting to deploy a commit to the itshover's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughAdds a TypeScript script to generate Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant npm as npm (registry:build)
participant gen as scripts/generate-registry.ts
participant idx as icons/index.ts
participant fs as file system (icons/*.tsx)
participant reg as registry.json
participant shd as shadcn build
participant out as public/r/*.json
participant fmt as prettier
Dev->>npm: npm run registry:build
npm->>gen: execute (npx tsx scripts/generate-registry.ts)
gen->>fs: scan icons/*.tsx (exclude index/types)
gen->>idx: parse ICON_LIST from icons/index.ts
gen->>gen: validate fs ↔ ICON_LIST (warn mismatches)
gen->>reg: write/update registry.json (auto-generated)
npm->>shd: shadcn build (reads registry.json)
shd->>out: generate per-icon JSON files in public/r/
npm->>fmt: prettier --write registry.json public/r
fmt->>out: format outputs
fmt-->>Dev: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
Comment |
25f6094 to
5451538
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
public/r/snapchat-icon.json (1)
9-10: Consider removingasynckeyword from thestartfunction.The
startcallback is declared asasyncbut doesn't useawaiton theanimate()calls, meaning animations run in parallel immediately. If parallel execution is intended, theasynckeyword is unnecessary and can be removed for clarity.🔎 Proposed refinement
If parallel animations are intended (current behavior):
- const start = useCallback(async () => { + const start = useCallback(() => { animate( ".ghost", { y: [0, -2, 0], scale: [1, 1.05, 1] }, { duration: 0.6, ease: "easeInOut" }, ); animate( ".arms", { scaleX: [1, 1.1, 1] }, { duration: 0.4, ease: "easeInOut" }, ); }, [animate]);Alternatively, if sequential animations are desired, add
await:- const start = useCallback(async () => { - animate( + const start = useCallback(async () => { + await animate( ".ghost", { y: [0, -2, 0], scale: [1, 1.05, 1] }, { duration: 0.6, ease: "easeInOut" }, ); - animate( + await animate( ".arms", { scaleX: [1, 1.1, 1] }, { duration: 0.4, ease: "easeInOut" }, ); }, [animate]);public/r/ghost-icon.json (1)
10-10: Consider wrapping animation functions inuseCallbackfor consistency.Other icons in this PR (gmail-icon, python-icon, instagram-icon, rosette-discount-icon) wrap their
startandstopfunctions inuseCallback. Additionally, thehandleHoverStart/handleHoverEndwrappers are redundant since they just delegate tostart()/stop().🔎 Suggested alignment with other icons
- const start = async () => { + const start = useCallback(async () => { // ... animation logic - }; + }, [animate]); - const stop = () => { + const stop = useCallback(() => { // ... animation logic - }; + }, [animate]); // Remove redundant wrappers and pass start/stop directly: - const handleHoverStart = () => { - start(); - }; - - const handleHoverEnd = () => { - stop(); - }; - return ( <motion.svg - onHoverStart={handleHoverStart} - onHoverEnd={handleHoverEnd} + onHoverStart={start} + onHoverEnd={stop}scripts/generate-registry.ts (3)
65-65: Hardcodedmotiondependency may not apply to all icons.Line 65 hardcodes
"motion"as a dependency for every icon. If a future icon doesn't use motion animations, this would add an unnecessary dependency.Consider detecting the actual dependencies by:
- Parsing the icon file's imports to determine if motion is used
- Or maintaining a configuration map for icons with different dependencies
- Or accepting that all icons in this project will use motion and document this requirement
For now, if all icons are expected to use motion, add a comment explaining this assumption:
return { name, type: "registry:ui", registryDependencies: [], + // All icons in this registry use motion for animations dependencies: ["motion"], devDependencies: [],
171-175: The_generatedfield will appear in the final JSON.Lines 172-173 add a
_generatedfield as a workaround for JSON's lack of comment support. While this works, the field will be present in the publishedregistry.jsonfile, which:
- Adds unnecessary bytes to the file served to users
- Is not part of the shadcn registry schema
- Could confuse tools or parsers expecting only schema-defined fields
Consider alternatives:
- Prepend a comment before running prettier: Write a temporary file with a
//comment, then let prettier format it. (Prettier preserves comments in JSON with thejsoncparser.)- Document in separate file: Add a README or CONTRIBUTING note instead.
- Keep as-is: If the extra ~100 bytes are acceptable and no issues arise from the extra field.
If keeping the current approach, consider using a more descriptive key like
$commentwhich is sometimes used in JSON schemas:- const registryWithNotice = { - _generated: - "AUTO-GENERATED FILE - DO NOT EDIT. Run 'npm run registry:build' to regenerate.", - ...registry, - }; + const registryWithNotice = { + $comment: + "AUTO-GENERATED - DO NOT EDIT. Run 'npm run registry:build' to regenerate.", + ...registry, + };
88-99: Regex parsing of TypeScript source is brittle—consider refactoring to AST parsing for long-term maintainability.The current regex approach
/name:\s*["']([^"']+)["']/gworks correctly for the existing codebase, but it has inherent fragility. While the hyphen filtering effectively distinguishes icon names from customProps, the regex would break if the ICON_LIST format changes (e.g., using template literals, adding line breaks within entries, or computed properties). Additionally, any quoted strings or comments in the file containing thename:pattern could theoretically be matched.For improved robustness, consider using a TypeScript AST parser as shown in the detailsbelow. This would be more maintainable and resilient to formatting changes. Alternatively, if the current regex approach is deemed sufficient for the project's needs, add explicit documentation of the expected format constraints and ensure test coverage validates the extraction logic.
AST parser alternative
import * as ts from "typescript"; function getIconListNames(): string[] { const indexPath = path.join(ICONS_DIR, "index.ts"); const sourceFile = ts.createSourceFile( indexPath, fs.readFileSync(indexPath, "utf-8"), ts.ScriptTarget.Latest, true ); const names: string[] = []; // Traverse AST to find ICON_LIST variable and extract name properties // This would be more robust than regex return [...new Set(names)]; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
ARCHITECTURE.mdCONTRIBUTING.mdicons/index.tspackage.jsonpublic/r/accessibility-icon.jsonpublic/r/ampersand-icon.jsonpublic/r/arrow-big-down-dash-icon.jsonpublic/r/arrow-big-down-icon.jsonpublic/r/arrow-down-0-1-icon.jsonpublic/r/brand-nextjs-icon.jsonpublic/r/coin-bitcoin-icon.jsonpublic/r/currency-bitcoin-icon.jsonpublic/r/currency-dollar-icon.jsonpublic/r/currency-ethereum-icon.jsonpublic/r/currency-euro-icon.jsonpublic/r/currency-rupee-icon.jsonpublic/r/discord-icon.jsonpublic/r/docker-icon.jsonpublic/r/facebook-icon.jsonpublic/r/figma-icon.jsonpublic/r/ghost-icon.jsonpublic/r/gmail-icon.jsonpublic/r/home-icon.jsonpublic/r/instagram-icon.jsonpublic/r/javascript-icon.jsonpublic/r/layout-sidebar-right-collapse-icon.jsonpublic/r/linkedin-icon.jsonpublic/r/mysql-icon.jsonpublic/r/nodejs-icon.jsonpublic/r/party-popper-icon.jsonpublic/r/pinterest-icon.jsonpublic/r/python-icon.jsonpublic/r/qrcode-icon.jsonpublic/r/registry.jsonpublic/r/rosette-discount-check-icon.jsonpublic/r/rosette-discount-icon.jsonpublic/r/slack-icon.jsonpublic/r/snapchat-icon.jsonpublic/r/spotify-icon.jsonpublic/r/star-icon.jsonpublic/r/telephone-icon.jsonpublic/r/typescript-icon.jsonpublic/r/vercel-icon.jsonregistry.jsonscripts/generate-registry.ts
🔇 Additional comments (43)
public/r/star-icon.json (2)
1-7: LGTM!The registry metadata is correctly structured with the appropriate schema reference and dependency on
motionfor the animation library.
13-17: LGTM!The type definitions are well-structured with helpful JSDoc comments. The
AnimatedIconHandleinterface provides a clean imperative API for controlling animations programmatically.public/r/mysql-icon.json (3)
1-6: Registry metadata looks good.The schema reference and basic structure align with shadcn's registry format. The naming convention (
mysql-icon) is consistent with the pattern.
15-15: Type definitions are well-structured.The interfaces are clearly defined with helpful JSDoc comments on
AnimatedIconProps. The optional properties with sensible defaults provide good flexibility.Note: The
startAnimation: () => voidreturn type inAnimatedIconHandleconflicts with the async implementation inmysql-icon.tsx(already flagged separately).
4-4: No issues found. The project uses Motion v12.23.26, which fully supports themotion/reactimport path and matches the dependency declaration.public/r/snapchat-icon.json (4)
1-6: LGTM! Registry metadata is properly structured.The schema, naming convention, and dependency declarations follow shadcn registry standards correctly.
14-15: LGTM! Type definitions are well-structured and documented.The
AnimatedIconPropsandAnimatedIconHandleinterfaces provide clear contracts with helpful JSDoc comments. They correctly match the component implementation.
7-20: Registry structure correctly bundles component with type definitions.The file array properly includes both the icon component and shared types, ensuring users can install the complete SnapchatIcon via shadcn CLI. This addresses the PR objective of making icon JSON files available at public registry URLs (fixing issue #27).
9-10: Import path is correct for Motion v12.23.26.The component correctly imports from
"motion/react", which is the official import path for Motion v12.23.26. No changes needed.CONTRIBUTING.md (2)
252-257: LGTM! Clear documentation of the three-step registry workflow.The updated documentation accurately describes the automated registry generation process. The explanation is clear and aligns with the PR's objectives to replace manual registry maintenance.
359-359: LGTM! Consistent terminology.The updated comment correctly reflects that
registry:buildnow performs both generation and building steps.public/r/typescript-icon.json (1)
1-20: LGTM! Well-implemented icon component.The
TypescriptIconfollows best practices:
- Properly uses
useCallbackto memoize animation handlers- Correctly implements
forwardRefanduseImperativeHandle- Animation sequences are well-structured with appropriate easing
- Consistent with the shadcn registry schema
public/r/vercel-icon.json (1)
1-20: LGTM! Excellent implementation.The
VercelIconis well-implemented with:
- Proper use of
useCallbackfor memoized animation handlers- Correct
forwardRefanduseImperativeHandleimplementation- Smooth animation sequence with appropriate easing curves
- Consistent use of
motion.svg(matches the pattern in most icons)public/r/arrow-big-down-dash-icon.json (1)
1-20: LGTM!The registry item follows the expected pattern: uses
motion/react, implements theforwardRefpattern with imperative handle, and includes the shared types. The animation logic for the arrow and dash elements is well-structured.public/r/party-popper-icon.json (1)
1-20: LGTM!The party-popper icon correctly implements the forwardRef pattern with imperative handle, uses
motion/react, and properly memoizes animation callbacks withuseCallback. Theoverflow: visibleon the SVG is appropriate for the confetti animation effect.public/r/arrow-down-0-1-icon.json (1)
1-20: LGTM!The icon correctly implements the forwardRef pattern with imperative handle, uses
motion/react, and the swap animation logic between.zeroand.oneelements is well-structured.public/r/discord-icon.json (1)
1-20: LGTM!The Discord icon correctly implements the forwardRef pattern with imperative handle, uses
motion/react, and properly setstransformOriginon the eye elements for correct scaling behavior. The animation sequence for eyes → mouth → body creates a nice effect.public/r/currency-bitcoin-icon.json (1)
1-20: LGTM!The Bitcoin icon correctly implements the forwardRef pattern with imperative handle and uses
motion/react. The sequentialpathLengthanimation creates a nice "drawing" effect. Usingmotion.divas a wrapper (instead ofmotion.svgdirectly) is a valid approach that works well with theuseAnimatescope.public/r/pinterest-icon.json (1)
1-20: LGTM!The Pinterest icon correctly implements the forwardRef pattern with imperative handle, uses
motion/react, and properly memoizes animation callbacks. The pin bounce and circle scale animations work well together.public/r/gmail-icon.json (1)
1-20: LGTM!The GmailIcon registry item is well-structured. The component correctly uses
useCallbackfor animation functions, properly destructures and applies allAnimatedIconProps, and the registry metadata is complete.public/r/python-icon.json (1)
1-20: LGTM!The PythonIcon registry item follows the established pattern correctly with proper
useCallbackusage and complete props handling.public/r/instagram-icon.json (1)
1-20: LGTM!The InstagramIcon registry item is correctly implemented. The use of a
motion.divwrapper around the SVG is appropriate for the animation scope.public/r/rosette-discount-icon.json (1)
1-20: LGTM!The RosetteDiscountIcon registry item is well-implemented with clear inline comments documenting the animation sequence.
public/r/currency-ethereum-icon.json (1)
1-20: LGTM!The registry item structure is correct and the embedded CurrencyEthereumIcon component follows the established pattern with proper forwardRef, imperative handle exposure, and sequential animation logic.
public/r/brand-nextjs-icon.json (1)
1-20: LGTM!The registry item is correctly structured. Note that this icon uses a regular
<svg>element with nativeonMouseEnter/onMouseLeaveevents (rather thanmotion.svgwith motion-specificonHoverStart/onHoverEnd), which is a valid approach sinceuseAnimate's scope works with any DOM element.public/r/spotify-icon.json (1)
1-20: LGTM!The SpotifyIcon implementation correctly uses
useCallbackfor memoization and properly handles infinite animations. Thestop()function will implicitly cancel therepeat: Infinityanimations by animating the same properties to their reset values.public/r/rosette-discount-check-icon.json (1)
1-20: LGTM!The RosetteDiscountCheckIcon implementation is well-structured with proper animation sequencing (badge rotation → checkmark drawing → bounce-back effect) and correctly uses
useCallbackfor memoization.public/r/home-icon.json (1)
1-20: LGTM!The HomeIcon has a well-designed animation sequence (roof drop → house scale → door reveal) with proper
transformOriginplacement for the door'sscaleYanimation.public/r/qrcode-icon.json (1)
1-20: LGTM!The QrcodeIcon features a sophisticated multi-phase animation with staggered delays and a continuous scanning effect. The
viewBox="0 0 32 32"appropriately accommodates the more complex icon geometry compared to simpler 24x24 icons.public/r/docker-icon.json (1)
1-20: LGTM!The DockerIcon implementation effectively creates a floating whale animation with coordinated movement of the whale body and container boxes. The
useCallbackhooks and animation patterns are consistent with other icons in the registry.public/r/currency-rupee-icon.json (1)
1-20: LGTM!The registry item follows the shadcn schema correctly. The embedded
CurrencyRupeeIconcomponent properly implements theforwardRefpattern withuseImperativeHandlefor animation control, and thetypes.tsdependency is correctly included.public/r/facebook-icon.json (1)
1-20: LGTM!The FacebookIcon registry item is well-structured. The component correctly uses
overflow: visibleto allow the thumbs-up emoji animation to extend beyond the SVG bounds. The use ofuseCallbackfor animation functions is a good practice for memoization.public/r/coin-bitcoin-icon.json (1)
1-20: LGTM!The CoinBitcoinIcon registry item correctly implements a 3D coin flip animation using
rotateYwithtransformStyle: "preserve-3d". Since this is a filled icon (usingfill={color}rather than stroke), omittingstrokeWidthfrom the destructured props is appropriate.public/r/slack-icon.json (1)
1-20: LGTM!The SlackIcon registry item implements a visually appealing staggered rotation animation across the four logo pieces. The use of
useCallbackfor animation functions and propertransformOriginsettings on eachmotion.gelement ensures smooth, centered rotations.ARCHITECTURE.md (1)
145-158: LGTM!Clear documentation of the two-step registry generation workflow. This section effectively explains how
generate-registry.tscreatesregistry.jsonfromicons/index.ts, and howshadcn buildthen generates the individual JSON files inpublic/r/.public/r/nodejs-icon.json (1)
1-20: Registry structure looks good.The registry item follows the shadcn schema correctly, declares the
motiondependency, and includes both the icon component and shared types file. The embedded React code usesforwardRef,useImperativeHandle, anduseCallbackappropriately.public/r/javascript-icon.json (1)
1-20: LGTM!The registry item is correctly structured and the embedded JavascriptIcon component properly uses
useCallbackfor animation handlers,forwardReffor the imperative handle, and follows the established pattern.public/r/figma-icon.json (1)
1-20: LGTM!The registry item follows the correct schema and the embedded FigmaIcon component properly uses
useCallbackfor animation handlers, maintains ref stability withuseImperativeHandle, and follows the established icon implementation pattern.icons/index.ts (4)
7-7: LGTM! New icon imports are properly structured.The new icon imports (AmpersandIcon, ArrowBigDownIcon, ArrowBigDownDashIcon, ArrowDown01Icon, BrandNextjsIcon, CoinBitcoinIcon) are correctly added and follow the existing import pattern.
Also applies to: 18-19, 21-21, 41-41, 58-58
238-242: LGTM! New ICON_LIST entries are properly structured.The new icon entries for ampersand-icon, arrow-big-down-icon, arrow-big-down-dash-icon, arrow-down-0-1-icon, and coin-bitcoin-icon are correctly formatted with appropriate names, icon components, and keywords.
Also applies to: 300-304, 305-309, 315-319, 517-529
1341-1341: LGTM! Array sorting ensures consistent ordering.The
.sort()call ensures ICON_LIST entries are alphabetically ordered by name, which is important for the registry generation and user experience.
417-429: No duplicate entries found — the brand-nextjs-icon entry appears only once in ICON_LIST.The two occurrences found are the import statement (line 41) and the single ICON_LIST entry (line 418). The AI summary's claim of duplicate entries in ICON_LIST is incorrect.
public/r/linkedin-icon.json (1)
1-20: LGTM! The motion library usage is correct.The registry JSON structure is well-formed and follows the correct schema. The LinkedIn icon component properly uses
forwardRef,useImperativeHandle, and motion animations with a clean API surface.The code correctly uses the
motion/reactimport path with the Motion package (npm name "motion") listed in dependencies, which is the proper configuration for this syntax.
9edda8e to
28dbd4e
Compare
|
@coderabbitai ignore |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
public/r/star-icon.json (1)
1-20: LGTM!The star icon implementation is well-structured with a nice fill/outline animation effect. The missing
useImperativeHandledependency array was already flagged in previous review and will be addressed separately.
🧹 Nitpick comments (6)
public/r/instagram-icon.json (1)
1-20: LGTM with a note on pattern consistency.The implementation works correctly, though it uses a
motion.divwrapper around a plain<svg>instead ofmotion.svgdirectly like other icons. This is functionally equivalent sinceuseAnimate's scope queries descendants regardless, but you may want to align with themotion.svgpattern used elsewhere for consistency.public/r/ghost-icon.json (1)
9-11: Consider usinguseCallbackfor animation handlers.Unlike other icons in this PR (e.g., RosetteDiscountIcon, HomeIcon), the
startandstopfunctions are not wrapped withuseCallback. This creates new function instances on every render, potentially causing unnecessary re-renders when passed as dependencies.🔎 Suggested consistency improvement
- const start = async () => { + const start = useCallback(async () => { // floating + wiggle ... - }; + }, [animate]); - const stop = () => { + const stop = useCallback(() => { animate(".ghost-body", { y: 0, rotate: 0, scaleX: 1 }, { duration: 0.2 }); animate(".ghost-eye", { scaleX: 1, scaleY: 1 }, { duration: 0.2 }); - }; + }, [animate]);Additionally, the
handleHoverStartandhandleHoverEndwrapper functions are unnecessary:- const handleHoverStart = () => { - start(); - }; - - const handleHoverEnd = () => { - stop(); - }; - return ( <motion.svg ref={scope} - onHoverStart={handleHoverStart} - onHoverEnd={handleHoverEnd} + onHoverStart={start} + onHoverEnd={stop}public/r/currency-rupee-icon.json (1)
9-11: Align with the established pattern for consistency.Similar to GhostIcon, this component lacks
useCallbackwrappers and uses redundant hover handler functions. Consider aligning with the pattern demonstrated in HomeIcon and FigmaIcon for better consistency across the icon set.🔎 Suggested consistency improvement
- const start = async () => { + const start = useCallback(async () => { await animate( ... ); - }; + }, [animate]); - const stop = () => { + const stop = useCallback(() => { animate( ... ); - }; + }, [animate]); - useImperativeHandle(ref, () => { - return { - startAnimation: start, - stopAnimation: stop, - }; - }); + useImperativeHandle(ref, () => ({ + startAnimation: start, + stopAnimation: stop, + })); - const handleHoverStart = () => { - start(); - }; - - const handleHoverEnd = () => { - stop(); - }; - return ( <motion.svg ref={scope} - onHoverStart={handleHoverStart} - onHoverEnd={handleHoverEnd} + onHoverStart={start} + onHoverEnd={stop}public/r/facebook-icon.json (1)
10-10: Reconsider emoji usage in SVG text element for cross-browser consistency.The FacebookIcon component includes a
<motion.text>element with the emoji "👍". This approach has several drawbacks:
- Browser inconsistency: Emoji rendering in SVG
<text>elements varies significantly across browsers and operating systems (different fonts, sizes, colors).- Accessibility: Screen readers may not announce the emoji appropriately within an SVG context.
- Icon library standards: Professional icon libraries typically avoid text content (especially emojis) in favor of pure path-based graphics for predictable rendering.
Recommended alternatives:
- Replace the emoji with an SVG path-based thumbs-up icon
- Remove the emoji element entirely if it's decorative
- If emoji feedback is essential, render it as HTML outside the SVG
🔎 Proposed refactor to remove emoji text element
<motion.path className="fb-path" d="M7 10v4h3v7h4v-7h3l1 -4h-4v-2a1 1 0 0 1 1 -1h3v-4h-3a5 5 0 0 0 -5 5v2h-3" style={{ transformOrigin: "center" }} /> - <motion.text - className="fb-like" - x="12" - y="2" - textAnchor="middle" - fontSize="10" - fill={color} - opacity={0} - > - 👍 - </motion.text>And remove the
.fb-likeanimation from the start/stop callbacks.public/r/arrow-big-down-dash-icon.json (1)
10-10: Icon ignorescolorandstrokeWidthprops fromAnimatedIconProps.The component only destructures
sizeandclassName, but hardcodesstroke="currentColor"andstrokeWidth="2". This is the same pattern flagged for other icons in issue #29 (item #2). Users won't be able to customize stroke color or width.🔎 Proposed fix
const ArrowBigDownDashIcon = forwardRef<AnimatedIconHandle, AnimatedIconProps>( - ({ size = 40, className = "" }, ref) => { + ({ size = 40, color = "currentColor", strokeWidth = 2, className = "" }, ref) => { const [scope, animate] = useAnimate(); // ... return ( <motion.svg // ... - stroke="currentColor" - strokeWidth="2" + stroke={color} + strokeWidth={strokeWidth} // ... >Note: This follows the same pattern documented in issue #29 for consistent prop handling across all icons.
public/r/brand-nextjs-icon.json (1)
1-20: Consider usingmotion.svgfor consistency with other animated icons.The component uses a regular
svgelement withonMouseEnter/onMouseLeavehandlers while containingmotion.pathchildren. Other icons in this PR (e.g.,javascript-icon,arrow-down-0-1-icon) usemotion.svgwithonHoverStart/onHoverEnd, which integrates better with motion's animation system and provides more consistent behavior.🔎 Suggested refactor for consistency
- <svg + <motion.svg ref={scope} - onMouseEnter={handleHoverStart} - onMouseLeave={handleHoverEnd} + onHoverStart={handleHoverStart} + onHoverEnd={handleHoverEnd} xmlns="http://www.w3.org/2000/svg" width={size} height={size}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
ARCHITECTURE.mdCONTRIBUTING.mdicons/index.tspackage.jsonpublic/r/accessibility-icon.jsonpublic/r/ampersand-icon.jsonpublic/r/arrow-big-down-dash-icon.jsonpublic/r/arrow-big-down-icon.jsonpublic/r/arrow-down-0-1-icon.jsonpublic/r/brand-nextjs-icon.jsonpublic/r/coin-bitcoin-icon.jsonpublic/r/currency-bitcoin-icon.jsonpublic/r/currency-dollar-icon.jsonpublic/r/currency-ethereum-icon.jsonpublic/r/currency-euro-icon.jsonpublic/r/currency-rupee-icon.jsonpublic/r/discord-icon.jsonpublic/r/docker-icon.jsonpublic/r/facebook-icon.jsonpublic/r/figma-icon.jsonpublic/r/ghost-icon.jsonpublic/r/gmail-icon.jsonpublic/r/home-icon.jsonpublic/r/instagram-icon.jsonpublic/r/javascript-icon.jsonpublic/r/layout-sidebar-right-collapse-icon.jsonpublic/r/linkedin-icon.jsonpublic/r/mysql-icon.jsonpublic/r/nodejs-icon.jsonpublic/r/party-popper-icon.jsonpublic/r/pinterest-icon.jsonpublic/r/python-icon.jsonpublic/r/qrcode-icon.jsonpublic/r/registry.jsonpublic/r/rosette-discount-check-icon.jsonpublic/r/rosette-discount-icon.jsonpublic/r/slack-icon.jsonpublic/r/snapchat-icon.jsonpublic/r/spotify-icon.jsonpublic/r/star-icon.jsonpublic/r/telephone-icon.jsonpublic/r/typescript-icon.jsonpublic/r/vercel-icon.jsonregistry.jsonscripts/generate-registry.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- package.json
- CONTRIBUTING.md
- public/r/coin-bitcoin-icon.json
- public/r/snapchat-icon.json
- public/r/qrcode-icon.json
- public/r/mysql-icon.json
- public/r/pinterest-icon.json
- public/r/docker-icon.json
- public/r/currency-dollar-icon.json
- public/r/linkedin-icon.json
- public/r/vercel-icon.json
- scripts/generate-registry.ts
- public/r/layout-sidebar-right-collapse-icon.json
- public/r/gmail-icon.json
- public/r/currency-euro-icon.json
- public/r/currency-bitcoin-icon.json
🔇 Additional comments (17)
ARCHITECTURE.md (1)
145-158: LGTM!The documentation clearly explains the two-step registry generation workflow and provides actionable instructions for running the build. This aligns well with the PR objectives of creating an automated, reproducible workflow.
public/r/nodejs-icon.json (1)
1-20: LGTM!The registry item follows the established pattern with proper forwardRef usage, useCallback-wrapped animation handlers, and correct motion/react integration. The
useImperativeHandlemissing dependency array is a known issue that will be addressed in a separate PR (per past review discussion).public/r/party-popper-icon.json (1)
1-20: LGTM!Good use of
overflow: visibleon the SVG to allow confetti animations to extend beyond the icon bounds. The animation keyframes create a nice burst effect appropriate for a party popper.public/r/discord-icon.json (1)
1-20: LGTM!Well-implemented Discord icon with appropriate animation sequencing—the
awaiton the mouth animation creates a pleasing cascade effect where the body bounce follows the facial animations.public/r/spotify-icon.json (1)
1-20: LGTM!The infinite wave animations create an engaging audio visualization effect. The
stop()function correctly resets the animated properties, and motion/react'sanimate()will cancel the previous infinite animations when applying the new values.public/r/home-icon.json (1)
9-11: LGTM! Well-structured animation implementation.The HomeIcon component demonstrates good practices:
- Proper use of
useCallbackfor animation handlers to prevent unnecessary re-renders- Direct attachment of animation functions to hover events without redundant wrappers
- Clean async animation sequencing with appropriate easing
This serves as a good reference pattern for the other icons in this PR.
public/r/figma-icon.json (1)
9-11: LGTM! Consistent implementation.The FigmaIcon follows the established best practices with proper
useCallbackusage and clean animation sequencing. The parallel animations on different layers create an effective visual effect.public/r/rosette-discount-icon.json (1)
14-16: No action needed. The embeddedicons/types.tsduplication across 191 registry files is intentional and follows shadcn's documented registry pattern. The CLI automatically deduplicates files by target path resolution ("icons/types.ts"), and skips updates when content is identical. This is the correct approach for shared files in shadcn registries.public/r/python-icon.json (1)
1-20: Icon implementation follows the standard pattern.The PythonIcon component implementation is consistent with the other animated icons in this PR. The animation logic (snake movement and eye scaling) is well-structured.
Note: This file is affected by the shared types duplication issue flagged in
rosette-discount-check-icon.json. Once that architectural issue is resolved, this icon entry will need the same registry dependency update.public/r/slack-icon.json (1)
1-20: Icon implementation follows the standard pattern with effective staggered animations.The SlackIcon component uses well-structured staggered delays (0.1s, 0.2s, 0.3s) for the four pieces, creating a pleasing sequential animation effect. The use of
<motion.g>for grouping animated SVG elements is appropriate.Note: This file is affected by the shared types duplication issue flagged in
rosette-discount-check-icon.json. Once that architectural issue is resolved, this icon entry will need the same registry dependency update.public/r/typescript-icon.json (1)
1-20: Icon registry entry is well-formed aside from shared issues.The TypescriptIcon registry structure is correct and follows the established pattern. Once the animation flicker issue above and the shared types duplication issue (flagged in
rosette-discount-check-icon.json) are resolved, this entry will be production-ready.public/r/rosette-discount-check-icon.json (1)
4-4: Motion dependency is correctly configured.The verification confirms:
- The package name "motion" is correct (rebranded from framer-motion)
- The
/reactexport path is valid and properly defined in the package- Current version 12.23.26 supports React integration
No action required.
public/r/arrow-big-down-icon.json (1)
1-20: LGTM!The registry item structure follows the shadcn schema correctly. The animation implementation is functional, and the previous
useCallbackdiscussion has been marked as out of scope for future work.public/r/arrow-down-0-1-icon.json (1)
1-20: LGTM!The registry item is well-structured and the swap animation logic is sound. The component properly animates elements in opposite directions to create a visual swap effect.
public/r/javascript-icon.json (1)
1-20: LGTM! Excellent implementation.This icon demonstrates the recommended pattern with
useCallbackfor animation handlers,motion.svgwithonHoverStart/onHoverEnd, and proper dependency management. The animation logic is clean and well-structured.icons/index.ts (2)
7-58: LGTM! Clean import additions.All six new icon imports follow the established naming conventions and are properly integrated into the module structure.
238-529: LGTM! Well-structured ICON_LIST entries.The new entries for
ampersand-icon,arrow-big-down-icon,arrow-big-down-dash-icon,arrow-down-0-1-icon,brand-nextjs-icon, andcoin-bitcoin-iconare complete with appropriate keywords that will help users discover these icons. The keywords are descriptive and cover relevant use cases (e.g., "sort, ascending, number, order" for arrow-down-0-1-icon).
✅ Actions performedReviews paused. |
✅ Review Feedback AddressedAll review comments from this PR have been tracked in issue #29 for resolution in future PRs: 🔗 Issue #29: Icon Registry - Fix styling bugs, code consistency, accessibility, and config issues The issue includes:
Thank you for the thorough review! 🙏 |
|
Ready for review @Abhijit-Jha |
|
LGTM |
Problem
Some icons (like linkedin-icon) were missing from
registry.jsonbecause it was manually maintained. This caused the shadcn CLI to fail with "not found" errors.Solution
Created
scripts/generate-registry.ts:registry.jsonNow when executing
npm run registry:build:registry.jsonfromicons/index.tspublic/r/registry.jsonandpublic/r/*.jsonfilesChanges
scripts/generate-registry.tsregistry:buildcommand (generate + build + format) inpackage.jsonCONTRIBUTING.md&ARCHITECTURE.mdfor new workflow documentation/iconsforwardRefstandard yet)Tests
Run
npm run registry:buildwith some icons aren't listed inICON_LIST(meaning doesn't show in website's search)Icons are showing fine in website (e.g. LinkedIn)
Test library
Fixes #27
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.