docs(ui): add design system guidelines#2252
docs(ui): add design system guidelines#2252cylewaitforit wants to merge 2 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! |
Co-authored-by: Alex Savelyev <91429106+alexdln@users.noreply.github.com>
cylewaitforit
force-pushed
the
sb-design-guidelines
branch
from
9929234 to
0bd8a72
Compare
📝 WalkthroughWalkthroughExtracted the Unocss theme from Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/design-system-guidelines.mdx (1)
19-19: Minor: Missing period at end of sentence.📝 Suggested fix
-Text and input fields should not exceed **768px** in width +Text and input fields should not exceed **768px** in width.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 903b1231-571d-4bfb-bcbc-c29d267176b3
📒 Files selected for processing (6)
.storybook/main.tsapp/colors.mdxapp/design-system-guidelines.mdxapp/typography.mdxuno.config.tsuno.theme.ts
|
|
||
| ## Colors | ||
|
|
||
| - Primary surfaces: **white** (`fg-fg`) |
There was a problem hiding this comment.
Likely typo: fg-fg should be fg.
The theme defines fg.DEFAULT which generates the utility class fg, not fg-fg. This appears to be a documentation error.
📝 Suggested fix
-- Primary surfaces: **white** (`fg-fg`)
+- Primary surfaces: **white** (`fg`)There was a problem hiding this comment.
Seems like coderabbit could be right. @alexdln Should this just be fg ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think so since we do stuff like text-fg and bg-fg - although I thought I'd saw fg-fg somewhere but now I can't find it
| @@ -0,0 +1,110 @@ | |||
| import { Meta, ColorPalette, ColorItem } from '@storybook/addon-docs/blocks' | |||
There was a problem hiding this comment.
is it common to put the story inside the file? Like should we have a colors.stories.ts?
There was a problem hiding this comment.
Good question.
For files that use Component Story Format(CSF) it should use the *.stories.* naming pattern.
For related or unattached mdx doc files they generally don't use that pattern. As can be seen in examples of the storybook docs on mdx.
If at some point we need to make sure some other tool doesn't load the mdx files we could probably switch to a *.stories.mdx naming pattern but that was historically a custom mdx with CSF type file that storybook removed in SB7.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/typography.mdx (1)
37-56: Deduplicate the repeated typography scale array.The
fontSizeslist is duplicated in both largeTypesetblocks, which makes future token updates easy to miss in one place.♻️ Suggested refactor
export const wind4FontSizes = { 'xs': '0.75rem', 'sm': '0.875rem', 'base': '1rem', 'lg': '1.125rem', 'xl': '1.25rem', '2xl': '1.5rem', '3xl': '1.875rem', '4xl': '2.25rem', '5xl': '3rem', } + +export const fullTypographyScale = [ + wind4FontSizes['5xl'], // 48px + wind4FontSizes['4xl'], // 36px + wind4FontSizes['3xl'], // 30px + wind4FontSizes['2xl'], // 24px + wind4FontSizes['xl'], // 20px + wind4FontSizes['lg'], // 18px + wind4FontSizes['base'], // 16px + wind4FontSizes['sm'], // 14px + wind4FontSizes['xs'], // 12px + theme.text['2xs'].fontSize, // 11px + theme.text['3xs'].fontSize, // 10px + theme.text['4xs'].fontSize, // 9px + theme.text['5xs'].fontSize, // 8px +] @@ <Typeset fontFamily={theme.font.sans} - fontSizes={[ - wind4FontSizes['5xl'], // 48px - wind4FontSizes['4xl'], // 36px - wind4FontSizes['3xl'], // 30px - wind4FontSizes['2xl'], // 24px - wind4FontSizes['xl'], // 20px - wind4FontSizes['lg'], // 18px - wind4FontSizes['base'], // 16px - wind4FontSizes['sm'], // 14px - wind4FontSizes['xs'], // 12px - theme.text['2xs'].fontSize, // 11px - theme.text['3xs'].fontSize, // 10px - theme.text['4xs'].fontSize, // 9px - theme.text['5xs'].fontSize, // 8px - ]} + fontSizes={fullTypographyScale} fontWeight={400} sampleText="The quick brown fox jumps over the lazy dog" /> @@ <Typeset fontFamily={theme.font.mono} - fontSizes={[ - wind4FontSizes['5xl'], // 48px - wind4FontSizes['4xl'], // 36px - wind4FontSizes['3xl'], // 30px - wind4FontSizes['2xl'], // 24px - wind4FontSizes['xl'], // 20px - wind4FontSizes['lg'], // 18px - wind4FontSizes['base'], // 16px - wind4FontSizes['sm'], // 14px - wind4FontSizes['xs'], // 12px - theme.text['2xs'].fontSize, // 11px - theme.text['3xs'].fontSize, // 10px - theme.text['4xs'].fontSize, // 9px - theme.text['5xs'].fontSize, // 8px - ]} + fontSizes={fullTypographyScale} fontWeight={400} sampleText="const greeting = 'Hello, World!';" />Also applies to: 71-90
|
|
||
| ## Line Height | ||
|
|
||
| Base line height is set to **1.6** for optimal readability. This provides comfortable spacing for multi-line text blocks while maintaining density for UI components. |
There was a problem hiding this comment.
Use the standard hyphenated term “line-height”.
At Line 154, “Base line height” reads better as “Base line-height” in typography docs.
📝 Suggested wording tweak
-Base line height is set to **1.6** for optimal readability. This provides comfortable spacing for multi-line text blocks while maintaining density for UI components.
+Base line-height is set to **1.6** for optimal readability. This provides comfortable spacing for multi-line text blocks while maintaining density for UI components.📝 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.
| Base line height is set to **1.6** for optimal readability. This provides comfortable spacing for multi-line text blocks while maintaining density for UI components. | |
| Base line-height is set to **1.6** for optimal readability. This provides comfortable spacing for multi-line text blocks while maintaining density for UI components. |
2 participants
🔗 Linked issue
🧭 Context
Codifies design system guidelines from @alexdln into the storybook docs.
📚 Description
Adds storybook doc pages for:
Moves theme from
uno.config.tsintouno.theme.tsin order for it to be consumed in the storybook docs.