Skip to content

chore(monorepo): 🤖 make "assets" as package icons#939

Open
punkbit wants to merge 16 commits intochore/monorepo-migrationfrom
chore/monorepo-assets-as-package
Open

chore(monorepo): 🤖 make "assets" as package icons#939
punkbit wants to merge 16 commits intochore/monorepo-migrationfrom
chore/monorepo-assets-as-package

Conversation

@punkbit
Copy link
Copy Markdown
Collaborator

@punkbit punkbit commented Mar 23, 2026

Why?

Make the assets a separate package of name icons.

⚠️ Depends on #938 (which should be merged first)

How?

  • Move files from click-ui to its own packages/icon
  • Create typescript conf
  • Create aliases
  • Support build scripts

Preview?

N/A

@punkbit punkbit marked this pull request as draft March 23, 2026 18:29
@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Mar 23, 2026

⚠️ No Changeset found

Latest commit: b0bc924

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 23, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 23, 2026

Claude finished @punkbit's task in 1m 24s —— View job


PR Review: chore(monorepo): 🤖 make "assets" as package icons

  • Read CLAUDE.md and conventions
  • Review changed files
  • Post final review

Good progress since the last review — the previously-flagged must-fix issues have been addressed. Here's the current state:


✅ Previously flagged items — now resolved

  • packages/icons/package.json convert script paths are correct (.scripts/js/convert-svg-to-react-component) ✓
  • PaymentProps is now re-exported from @clickhouse/click-ui (line 35) ✓
  • getAssetType([]) correctly calls process.exit(1) with an error message rather than crashing ✓

💡 Suggestions (non-blocking)

  • Stale TODO linespackages/icons/.scripts/js/convert-svg-to-react-component:3-4 still reads "TODO: Move to .scripts/js once PR chore: 🤖 clear scss setup (technical debt) #784 is merged" but the file is already at .scripts/js/. Safe to remove both lines. Fix this →

  • IconName type duplicationpackages/click-ui/src/components/Icon/Icon.types.ts:16 defines a local IconName as (typeof ICON_NAMES)[number] (a runtime-derived union from Object.keys(IconsLight)) while @clickhouse/icons also exports a static string-union IconName. The TODO at that line acknowledges this — worth a follow-up to settle on a single canonical type to avoid consumer confusion.

  • Missing changeset@clickhouse/icons is a new public package. Per CONVENTIONS.md, yarn changeset:add should be run before merging: at minimum a minor entry for @clickhouse/icons (new package) and a patch for @clickhouse/click-ui (internal refactor with re-exports).


Everything else looks solid — dual ESM/CJS build, sideEffects: false, granular sub-path exports, the config.ts deprecation/alias system, and the re-export layer in @clickhouse/click-ui are all well done. 🙌

@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 23, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 23, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Mar 23, 2026
@punkbit punkbit marked this pull request as ready for review March 24, 2026 14:37
Copy link
Copy Markdown
Collaborator

@ariser ariser left a comment

Choose a reason for hiding this comment

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

Same as the other PRs, looking at the general idea and green CI, trusting that everyting that should be moved is actually properly moved.

@punkbit
Copy link
Copy Markdown
Collaborator Author

punkbit commented Mar 27, 2026

Same as the other PRs, looking at the general idea and green CI, trusting that everyting that should be moved is actually properly moved.

Yes, should be, but any revisions occurring after testing should cause a revision. Thanks!

@punkbit punkbit force-pushed the chore/monorepo-migration branch from 6f98dee to f2d1e91 Compare March 31, 2026 10:52
@punkbit punkbit force-pushed the chore/monorepo-assets-as-package branch from f975b48 to aca751f Compare March 31, 2026 10:58
@punkbit punkbit force-pushed the chore/monorepo-migration branch 2 times, most recently from e666464 to c441cdc Compare April 2, 2026 11:52
@punkbit punkbit force-pushed the chore/monorepo-assets-as-package branch from aca751f to 8074a6b Compare April 2, 2026 12:23
@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

✅ Preview URL: https://click-a9sxmoq8a-clickhouse.vercel.app

Built from commit: f944cc70dae73012e990efa5088396bc40582da6

@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 2, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 2, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 2, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 2, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 2, 2026
@ClickHouse ClickHouse deleted a comment from github-actions bot Apr 2, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

Claude finished @punkbit's task in 2m 50s —— View job


PR Review: chore(monorepo): 🤖 make "assets" as package icons

  • Read CLAUDE.md and conventions
  • Review changed files and diff
  • Post final review

Nice work on the extraction — the package structure, dual ESM/CJS build, sideEffects: false, granular sub-path exports, and the re-export layer in @clickhouse/click-ui are all solid. One must-fix remains before merging.


🚨 Must Fix

  • Missing changeset@clickhouse/icons is a new public package and @clickhouse/click-ui has re-exports added/removed. Per CONVENTIONS.md, run yarn changeset:add before merging: at minimum a minor entry for @clickhouse/icons (new package) and a patch for @clickhouse/click-ui (re-export changes). No changeset files for either package were found in .changeset/. Fix this →

💡 Suggestions (non-blocking)

  • Stale TODO comments (flagged last review, still present)packages/icons/.scripts/js/convert-svg-to-react-component:3-4 still reads "TODO: Move to .scripts/js once PR chore: 🤖 clear scss setup (technical debt) #784 is merged" but the file is already at .scripts/js/. Safe to remove both lines. Fix this →

  • Popover_Arrow export namingpackages/icons/src/index.ts:9 exports Popover_Arrow with an underscore (export { default as Popover_Arrow }), inconsistent with the rest of the PascalCase public API. Consider renaming to PopoverArrow (with a deprecated re-export for the old name if consumers already depend on it).

  • Dual IconProps definitions@clickhouse/icons exports an IconProps (basic SVG props + theme/size) while packages/click-ui/src/components/Icon/Icon.types.ts:17 defines a local IconProps (with state, color, size, name: IconName). They share the same name but are different shapes. The click-ui version is not re-exported publicly so it won't bite consumers directly, but it may confuse contributors reading the source. A comment clarifying the distinction would help.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants