feat(field): add typography and foreground props to Field.Label #399
Conversation
🦋 Changeset detectedLatest commit: 6163f62 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @MaxLee-dev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| import type { Foregrounds } from '~/styles/mixins/foreground.css'; | ||
| import { foregrounds } from '~/styles/mixins/foreground.css'; | ||
| import { type Typography, typography } from '~/styles/mixins/typography.css'; |
There was a problem hiding this comment.
import 구문의 일관성을 개선하고, typography 함수와 prop 이름의 충돌을 해결하기 위해 다음을 제안합니다.
foregrounds와Foregrounds타입을 하나의 import 구문으로 합칩니다.typography함수를typographyRecipe와 같이 alias하여 prop과의 이름 충돌을 피합니다.
| import type { Foregrounds } from '~/styles/mixins/foreground.css'; | |
| import { foregrounds } from '~/styles/mixins/foreground.css'; | |
| import { type Typography, typography } from '~/styles/mixins/typography.css'; | |
| import { type Foregrounds, foregrounds } from '~/styles/mixins/foreground.css'; | |
| import { type Typography, typography as typographyRecipe } from '~/styles/mixins/typography.css'; |
References
- 스타일 가이드 3.1.2는 외부 패키지에 대한 네임스페이스 사용을 권장하지만, 이 원칙을 적용하여 내부 모듈에서도
as키워드로 이름 충돌을 해결하고 코드 명확성을 높일 수 있습니다. (link)
There was a problem hiding this comment.
To maintain consistency with the Text Component and typography, it seems unnecessary to modify it.
| const { | ||
| typography: typographyStyle = 'body2', | ||
| foreground = 'normal-100', | ||
| className, | ||
| ...componentProps | ||
| } = resolveStyles(props); | ||
|
|
||
| return ( | ||
| <BaseField.Label ref={ref} className={clsx(styles.label, className)} {...componentProps} /> | ||
| <BaseField.Label | ||
| ref={ref} | ||
| className={clsx( | ||
| styles.label, | ||
| typography({ style: typographyStyle }), | ||
| foregrounds({ color: foreground }), | ||
| className, | ||
| )} | ||
| {...componentProps} | ||
| /> |
There was a problem hiding this comment.
앞선 제안에 따라 typography 함수를 typographyRecipe로 alias했다면, 더 이상 prop 이름을 변경할 필요가 없습니다. 코드를 다음과 같이 간소화할 수 있습니다.
| const { | |
| typography: typographyStyle = 'body2', | |
| foreground = 'normal-100', | |
| className, | |
| ...componentProps | |
| } = resolveStyles(props); | |
| return ( | |
| <BaseField.Label ref={ref} className={clsx(styles.label, className)} {...componentProps} /> | |
| <BaseField.Label | |
| ref={ref} | |
| className={clsx( | |
| styles.label, | |
| typography({ style: typographyStyle }), | |
| foregrounds({ color: foreground }), | |
| className, | |
| )} | |
| {...componentProps} | |
| /> | |
| const { | |
| typography = 'body2', | |
| foreground = 'normal-100', | |
| className, | |
| ...componentProps | |
| } = resolveStyles(props); | |
| return ( | |
| <BaseField.Label | |
| ref={ref} | |
| className={clsx( | |
| styles.label, | |
| typographyRecipe({ style: typography }), | |
| foregrounds({ color: foreground }), | |
| className, | |
| )} | |
| {...componentProps} | |
| /> | |
| ); |
|
✅ All tests passed!
Click here if you need to update snapshots. |
Add typography and foreground props to Field.Label.
| const { | ||
| typography: typographyStyle = 'body2', | ||
| foreground = 'normal-100', | ||
| className, | ||
| ...componentProps | ||
| } = resolveStyles(props); |
There was a problem hiding this comment.
All other components manage default values for variants in their .css.ts files. So I did some digging to remove the variant defaults defined in Field.Label's .tsx.
First, I considered importing the typography recipe from field.css.ts and directly composing a new recipe. However, since the existing recipe only returns a class name, this approach seems impossible.
The second method involves exporting only the variants object from the typography.css.ts file. Technically, this approach seems feasible.
// typography.css.ts
export const typographyVariants = {
display1: layerStyle('components', {
lineHeight: vars.typography.lineHeight['1000'],
letterSpacing: vars.typography.letterSpacing['400'],
fontSize: vars.typography.fontSize['1000'],
fontWeight: vars.typography.fontWeight[800],
}),
// ...
};
export const typography = recipe({
defaultVariants: { style: 'body1' },
variants: {
style: typographyVariants,
},
});
// field.css.ts
export const label = recipe({
defaultVariants: { foreground: 'normal-200', typography: 'body1' },
variants: {
foreground: foregroundVariants,
typography: typographyVariants,
},
});This approach seems like it would completely separate style concerns from the React component file. However, since it involves passing variants back and forth, it doesn't feel 100% intuitive, so it might be good to discuss this together!
There was a problem hiding this comment.
For each typo, values were created as objects and exported so they could be used in the recipe declaration of the component where they are actually used.
…t/field-typography-foreground-props
| }, | ||
| }); | ||
|
|
||
| export type ForegroundVariants = typeof foregroundVariants; |
There was a problem hiding this comment.
Just curious. What purpose is this type being exported for??
There was a problem hiding this comment.
I had intended to specify the type of props to be passed to Field.Label, but since I added it using the RecipeVariants<> utility type, it seemed unnecessary and I removed it.
…t/field-typography-foreground-props
Summary
typographyandforegroundprops toField.Labelcomponent for customizable stylingtypography='body2',foreground='normal-100'(matches previous hardcoded behavior)Changes
field.tsx: Addtypographyandforegroundprops with TypeScript typesfield.css.ts: Simplify label style to only contain layout propertiesUsage