Skip to content

chore: 🤖 add new icons expand-all and collapse-all#995

Merged
kunalpanchal merged 1 commit intomainfrom
add-new-icons-collapse-expand
Apr 15, 2026
Merged

chore: 🤖 add new icons expand-all and collapse-all#995
kunalpanchal merged 1 commit intomainfrom
add-new-icons-collapse-expand

Conversation

@kunalpanchal
Copy link
Copy Markdown
Collaborator

Why?

Add new Icons collapse-all, expand-all

Tickets?

resolves #988

Contribution checklist?

  • You've done enough research before writing
  • You have reviewed the PR
  • The commit messages are detailed
  • The build command runs locally
  • Assets or static content are linked and stored in the project
  • For documentation, guides or references, you've tested the commands

Security checklist?

  • All user inputs are validated and sanitized
  • No usage of dangerouslySetInnerHTML
  • Sensitive data has been identified and is being protected properly
  • Build output contains no secrets or API keys

Preview?

Optionally, provide a demo or a preview url here

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 15, 2026

🦋 Changeset detected

Latest commit: 4db9536

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clickhouse/click-ui Patch

Not sure what this means? Click here to learn what changesets are.

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 15, 2026

Claude finished @kunalpanchal's task in 1m 26s —— View job


PR Review

Nice work adding these two icons and wiring them up through the type system, icon maps, and registry all at once! The structure is clean and consistent with the rest of the codebase. One issue needs addressing before merge.


🚨 Must Fix

  • Collapse-All.tsx & Expand-All.tsx: Both icons define <clipPath id="a">. Since these are inline SVGs, all element IDs live in the same HTML document scope. When both icons appear on the same page simultaneously, the DOM will have two <clipPath id="a"> elements, causing an ID collision — the second icon will reference the wrong clip path definition and render incorrectly.

    Looking at other icons with clip paths in this repo (e.g. Tree-Structure.tsx uses id="clip0_7969_2214", Query.tsx uses id="clip0_4716_4565"), they use unique Figma-derived IDs to avoid exactly this problem.

    Fix: use unique IDs in each file, e.g. id="clip-collapse-all" and id="clip-expand-all", and update the url(#...) references to match.

    Fix this →


💡 Suggestions (non-blocking)

  • Changeset missing: The changeset bot flagged this. Per CONVENTIONS.md, new icons added to the public API should have a changeset. Since this is a patch-level addition, it can be brief — run yarn changeset:add to generate one.

  • Color: Existing icons use stroke="#161517" (near-black). These new icons are filled-style and use fill="#000" (pure black). Minor inconsistency, and the icon design dictates the approach — just something to be aware of if icons are expected to visually match across styles.


  • Read conventions
  • Read changed files
  • Analyze and post review

@kunalpanchal kunalpanchal force-pushed the add-new-icons-collapse-expand branch from c4ae6bb to 4db9536 Compare April 15, 2026 09:58
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 15, 2026

Claude finished @kunalpanchal's task in 41s —— View job


PR Review

Great follow-up — all previous issues have been resolved! 🙌

✅ clipPath ID collision fixedCollapse-All.tsx now uses id="clip-collapse-all" and Expand-All.tsx uses id="clip-expand-all", eliminating the DOM ID collision when both icons appear on the same page.

✅ Changeset added.changeset/add-collapse-all-expand-all-icons.md is present with the correct patch bump.

✅ Fill color corrected — both icons now use fill="#161517" (near-black, consistent with the rest of the icon set) instead of fill="#000".

The type registration, icon maps (IconsLight, IconsDark), and the icon components themselves are all clean and consistent with the rest of the codebase.

✅ Looks great — ready to merge!


  • Read changed files
  • Check if clipPath ID collision was fixed
  • Post review

@workflow-authentication-public
Copy link
Copy Markdown
Contributor

📚 Storybook Preview Deployed

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

Built from commit: 51d06ca4bdf4c439b1a59d85665b8a71eb6d138b

@kunalpanchal kunalpanchal marked this pull request as ready for review April 15, 2026 10:02
@kunalpanchal kunalpanchal requested a review from punkbit April 15, 2026 10:03
@punkbit punkbit changed the title chore: add icons - expand-all, collapse-all chore: 🤖 add new icons expand-all and collapse-all Apr 15, 2026
Copy link
Copy Markdown
Contributor

@punkbit punkbit left a comment

Choose a reason for hiding this comment

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

@kunalpanchal Thank you!

Just out of curiosity, how did using the new automated asset tools feel compared to doing this manually in the past?

@kunalpanchal
Copy link
Copy Markdown
Collaborator Author

@kunalpanchal Thank you!

Just out of curiosity, how did using the new automated asset tools feel compared to doing this manually in the past?

Heya, the overall experience of using click-ui in general after a while feels amazing ^_^
Both the changeset generation and icon conversion to tsx felt seemless. Great work 💯

@kunalpanchal kunalpanchal merged commit 0d2aa03 into main Apr 15, 2026
10 checks passed
@kunalpanchal kunalpanchal deleted the add-new-icons-collapse-expand branch April 15, 2026 10:20
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.

New icons "expand all" and "collapse all"

2 participants