docs(ui): add ButtonGroup stories to storybook#1964
docs(ui): add ButtonGroup stories to storybook#1964cylewaitforit wants to merge 8 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Storybook configuration and button stories: adds the storybook-i18n addon and LOCALE_CHANGED handling, creates a dedicated theme module (npmxDark) used by manager and preview, injects docs theme and a docs canvas background CSS, and removes a fault-tolerant wrapper around vue-docgen-plugin. Moves Button stories from a deleted Base.stories.ts into new ButtonBase.stories.ts and ButtonGroup.stories.ts, and adds component name declarations via defineOptions in Button Base and Group components. Updates package.json and pnpm-workspace.yaml to include storybook-i18n and extends knip.ts ignoreDependencies. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.storybook/preview.ts (1)
85-85: Avoid usinganytype for i18n instance.The
i18nInstanceis typed asany, which bypasses TypeScript's type checking. Consider using the appropriate type fromvue-i18nor at minimum a more descriptive interface.🛠️ Suggested improvement
- let i18nInstance: any = null + let i18nInstance: { setLocale: (locale: string) => void } | null = null
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b33c7bfb-2b5b-4119-9765-c992b32e7ac6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.storybook/main.ts.storybook/manager.ts.storybook/preview-head.html.storybook/preview.ts.storybook/theme.tsapp/components/Button/Base.stories.tsapp/components/Button/Base.vueapp/components/Button/Button.stories.tsapp/components/Button/ButtonGroup.stories.tsapp/components/Button/Group.vuepackage.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- app/components/Button/Base.stories.ts
| <style> | ||
| /* Override docs story canvas background to match npmx theme */ | ||
| .docs-story { | ||
| background-color: var(--bg, oklch(0.171 0 0)) !important; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
The docs canvas background never follows the selected theme.
Line 4 depends on --bg, but Storybook never sets that variable in the provided setup: .storybook/preview.ts:62-82 only updates --accent-color, and .storybook/theme.ts:1-12 sets appContentBg for the manager UI rather than a CSS custom property. As a result, this always falls back to the dark value, so Light theme docs still render on a dark canvas. Please either initialise --bg in Storybook or switch this rule to a selector that keys off data-theme directly.
There was a problem hiding this comment.
The backgrounds in the autodocs pages look correct with this when viewing the light versions of components and removing this makes them not display correctly. Would love to find a way to fix this in the storybook theme overrides instead of needing this preview-head file but at this time have been unsuccessful.
There was a problem hiding this comment.
@cylewaitforit, thanks for the context! This is actually a well-known Storybook limitation. The dark theme is applied correctly in the canvas view but not in the Docs tab — the DOM inside Docs has extra wrappers and inline styles that override the background and theme styles. This is a tracked open bug (#30928) in Storybook, so your preview-head.html workaround is reasonable and pragmatic.
For a cleaner alternative (when you want to revisit), the closest officially-documented approach is:
-
Use
parameters.docs.themewith anappContentBgvalue. Storybook Docs uses the same theme system as Storybook's UI but is themed independently from the main UI. The default theme for Docs is always the "light" theme, regardless of the main UI theme. You can pass a custom theme specifically to docs in.storybook/preview.ts:import { npmxDark } from './theme'; // in preview parameters: docs: { theme: npmxDark, // applies appContentBg to the docs page wrapper }
However, this styles the docs page background, not the individual
.docs-storycanvas iframes — which is whypreview-head.htmlis still needed for the story canvas itself. -
Acknowledge the
!importantrisk: Sincepreview-head.htmluses!importantto override.docs-story, it will prevent any future per-story background customisation. If you later need story-specific backgrounds, you'd need to revisit this rule.
Given the upstream bug, the current preview-head.html approach is a valid and widely-used workaround. I'm withdrawing my original concern — the fix is intentional and appropriate for the current state of Storybook.
(ﾐ^ ◡ ^ﾐ) ✦ noted for future reviews!
✏️ Learnings added
Learnt from: cylewaitforit
Repo: npmx-dev/npmx.dev PR: 1964
File: .storybook/preview-head.html:1-6
Timestamp: 2026-03-06T17:44:59.085Z
Learning: In the npmx.dev project, the `.storybook/preview-head.html` file sets `background-color` on `.docs-story` via the `--bg` CSS custom property to fix the Autodocs canvas background. This is an intentional workaround for a known Storybook bug (storybookjs/storybook#30928) where `addon-themes` and `parameters.docs.theme` do not apply to the individual story canvas iframes in the Autodocs tab. Removing this file causes the autodocs pages to not display correctly.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 1042
File: app/assets/main.css:120-129
Timestamp: 2026-02-06T11:09:31.529Z
Learning: In the npmx.dev project's app/assets/main.css, the `:root[data-theme='light'][data-bg-theme='black']` variant intentionally only overrides `--bg-subtle` and reuses the default light theme values for `--bg`, `--bg-muted`, and `--bg-elevated`. This is by design because the default light theme is mostly neutral, so selective override is sufficient for the black theme variant.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3e058ab-44b3-47cc-ab0d-2d13215a73a5
📒 Files selected for processing (1)
app/components/Button/ButtonBase.stories.ts
| export const Default: Story = { | ||
| args: { | ||
| default: 'Button Text', | ||
| }, | ||
| } |
There was a problem hiding this comment.
Default story may not render slot content without a render function.
All other stories use explicit render functions with templates to handle slot content, but Default relies on args.default without a render function. In Vue Storybook, this pattern typically requires either a render function or specific argTypes configuration to pass slot content properly.
Consider adding a render function for consistency and to ensure the button text displays:
Proposed fix
export const Default: Story = {
- args: {
- default: 'Button Text',
- },
+ render: args => ({
+ components: { ButtonBase },
+ setup() {
+ return { args }
+ },
+ template: `<ButtonBase v-bind="args">Button Text</ButtonBase>`,
+ }),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const Default: Story = { | |
| args: { | |
| default: 'Button Text', | |
| }, | |
| } | |
| export const Default: Story = { | |
| render: args => ({ | |
| components: { ButtonBase }, | |
| setup() { | |
| return { args } | |
| }, | |
| template: `<ButtonBase v-bind="args">Button Text</ButtonBase>`, | |
| }), | |
| } |
There was a problem hiding this comment.
Keeping the default story with the default render allow to someone viewing the storybook to change the story to alternative text in place of Button Text. The other stories use the template render in order for the internationalization to work.
There was a problem hiding this comment.
@cylewaitforit, that makes perfect sense! Using args.default without a render function is the correct approach here — it exposes the slot content as a Storybook control, allowing users to interactively edit the button text directly from the controls panel. The render function approach in the other stories is needed for i18n to work, but would prevent that interactive editing for the Default story. Thanks for the clarification!
ʕ •`ᴥ´• ʔ
✏️ Learnings added
Learnt from: cylewaitforit
Repo: npmx-dev/npmx.dev PR: 1964
File: app/components/Button/ButtonBase.stories.ts:21-25
Timestamp: 2026-03-06T18:16:31.538Z
Learning: In the npmx.dev project's ButtonBase.stories.ts (app/components/Button/ButtonBase.stories.ts), the `Default` story intentionally uses `args.default` without a render function so that Storybook users can interactively edit the slot content (button text) via the controls panel. Other stories (e.g. Primary, Secondary, etc.) use explicit render functions with templates specifically to support storybook-i18n internationalization. Do not flag the `Default` story's lack of a render function as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1335
File: app/components/Compare/FacetSelector.vue:72-78
Timestamp: 2026-02-10T15:47:40.710Z
Learning: In the npmx.dev project, the `ButtonBase` component (app/components/ButtonBase.vue or similar) provides default `border border-border` styles. When styling ButtonBase instances, avoid adding duplicate `border` classes as this triggers the HTML validator error `no-dup-class` and fails CI.
Learnt from: cylewaitforit
Repo: npmx-dev/npmx.dev PR: 1964
File: .storybook/preview-head.html:1-6
Timestamp: 2026-03-06T17:44:59.085Z
Learning: In the npmx.dev project, the `.storybook/preview-head.html` file sets `background-color` on `.docs-story` via the `--bg` CSS custom property to fix the Autodocs canvas background. This is an intentional workaround for a known Storybook bug (storybookjs/storybook#30928) where `addon-themes` and `parameters.docs.theme` do not apply to the individual story canvas iframes in the Autodocs tab. Removing this file causes the autodocs pages to not display correctly.
|
Could you mark this as ready for review when you are? Then I'll take a look! |
57becd4 to
a923dd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Button/ButtonGroup.stories.ts (1)
26-106: Reduce the duplicated story markup.Each story now maintains the same example twice: once in
docs.source.codeand again inrender().template. That is easy to drift on the next edit and makes these stories more expensive to maintain than they need to be. A small helper or shared story data would keep the docs snippet and live render in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4a594fa-72da-4dbd-b9df-2f2dfca19696
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.storybook/main.ts.storybook/manager.ts.storybook/preview-head.html.storybook/preview.ts.storybook/theme.tsapp/components/Button/Base.stories.tsapp/components/Button/Base.vueapp/components/Button/ButtonBase.stories.tsapp/components/Button/ButtonGroup.stories.tsapp/components/Button/Group.vueknip.tspackage.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- app/components/Button/Base.stories.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- .storybook/theme.ts
- app/components/Button/Group.vue
- knip.ts
- pnpm-workspace.yaml
- app/components/Button/ButtonBase.stories.ts
- .storybook/main.ts
- app/components/Button/Base.vue
- package.json
| // Replace the built-in vue-docgen plugin with a fault-tolerant version. | ||
| // vue-docgen-api can crash on components that import types from other | ||
| // .vue files (it tries to parse the SFC with @babel/parser as plain TS). | ||
| // This wrapper catches those errors so the build doesn't fail. | ||
| const docgenPlugin = config.plugins?.find( | ||
| (p): p is Extract<typeof p, { name: string }> => | ||
| !!p && typeof p === 'object' && 'name' in p && p.name === 'storybook:vue-docgen-plugin', | ||
| ) | ||
|
|
||
| if (docgenPlugin && 'transform' in docgenPlugin) { | ||
| const hook = docgenPlugin.transform | ||
| // Vite plugin hooks can be a function or an object with a `handler` property | ||
| const originalFn = typeof hook === 'function' ? hook : hook?.handler | ||
| if (originalFn) { | ||
| const wrapped = async function (this: unknown, ...args: unknown[]) { | ||
| try { | ||
| return await originalFn.apply(this, args) | ||
| } catch { | ||
| return undefined | ||
| } | ||
| } | ||
| if (typeof hook === 'function') { | ||
| docgenPlugin.transform = wrapped as typeof hook | ||
| } else if (hook) { | ||
| hook.handler = wrapped as typeof hook.handler | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm very curious to hear why this is no longer necessary? 🤔 @cylewaitforit
There was a problem hiding this comment.
Why it's not needed I am not sure, while troubleshooting getting the autodocs to generate from the jsdoc comments for ButtonBase and ButtonGroup I removed it. Builds still worked.
Since it said it was needed to get builds to not fail, I didn't feel like it needed to be added it back.
There was a problem hiding this comment.
Thinking about it more, it's probably because these two components don't import their types from other .vue files.
Maybe that is okay, since I believe @danielroe added that as a known limitation.
Line 997 in 3de681d
There was a problem hiding this comment.
(I am also not married to keeping this removal in this PR. I'm all for deleting code but if it feels out of scope here happy to discuss in a smaller scoped PR.)
🔗 Linked issue
Part of #1841
🧭 Context
Adds stories for ButtonGroup
Preview npmx Storybook on Chromatic
📚 Description