Lyric UI Setup - #217
Conversation
justincorrigible
left a comment
justincorrigible
left a comment
There was a problem hiding this comment.
great progress thus far, and I'm happy to see a project actively adopting the new pattern and libraries! 👏 left a couple pieces of feedback here and there
one in general: the checklist has "Updated Tests" checked ("Unit and integration tests have been added that describe... the features that were added"), but I don't see a test file for the new Button component or cn utility, only button.stories.tsx (Storybook, not an automated assertion).
Is a stories file the intended test coverage for UI primitives in this repo's convention, or should that box stay unchecked until actual tests land?
| rollupOptions: { | ||
| external: ['react', 'react-dom', 'react/jsx-runtime'], | ||
| output: { | ||
| banner: "import './lyric-ui.css';", |
There was a problem hiding this comment.
this auto-injects a CSS import/require at the top of both bundles (i.e. MJS and CJS).
lyric-ui.cjs starts with require("./lyric-ui.css"), which will throw in plain Node because require() can't parse .css files... any consumer without a bundler-level CSS loader breaks immediately on require('@overture-stack/lyric-ui').
beyond that, this also creates ambiguity with the ./dist/lyric-ui.css path exported separately in package.JSON. if consumers were to either rely on the auto-injected import or import the CSS manually, or is even "doing both" meant to be harmless? tl;dr: worth documenting the intended consumption model, or dropping the banner if the manual export path is the real one.
There was a problem hiding this comment.
Ahhhh ok i see the issue.
My question now is what solution is ideal going forward? Do we want to auto inject the css or do we want to direct consumers to manually import the css themselves? Perhaps we could do both, only auto-inject for ESM but not CJS?
| @@ -0,0 +1,4 @@ | |||
| import '@/style.css'; | |||
There was a problem hiding this comment.
this import resolves fine internally via this package's own @/* path alias (in tsconfig), but tsc's declaration emit preserves it as is: dist/index.d.ts ships with import '@/style.css'; still in it... that alias means nothing to an external consumer, and doesn't even match the real output filename (lyric-ui.css, not style.css)... needs transformers.
while most consumers won't notice because skipLibCheck: true is a common default, this is a real defect in the published artifact. the way we handle this elsewhere is like so
- default is a depricated value in tailwindv4
2 participants
Summary
Lyric UI setup using tailwind,shadcn, react+vite and storybook.
Issues
Description of Changes
Packages
You will need to run
pnpm ito run the project.Readiness Checklist
.env.schemafile and documented in the README