-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(Text): a new component for working with typography #141
feat(Text): a new component for working with typography #141
Conversation
Preview is ready. |
faa5565
to
92f4a33
Compare
92f4a33
to
afd6c03
Compare
* <span className={textStyles}>some text</span> | ||
* ``` | ||
*/ | ||
export const Text: React.FC<TextProps> = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think forfardRef
should be added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need to use any type of ref things with this component. If you want to do this just use text
utility and certain element.
for example:
const Component = () => {
const ref = React.useRef<HTMLDivElement>(null);
return (
<div ref={ref} className={text()}>
some text
</div>
);
};
The keyword here is explicit
usage of ref
src/components/Text/Text.tsx
Outdated
}) => { | ||
return ( | ||
<Tag | ||
className={text({type, ellipsis}, color ? colorText({color}, className) : className)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to pass className
to colorText
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's useful if you want to add additional class to element. This technic called bem mix
afd6c03
to
e5f5ae8
Compare
/ping |
src/components/Text/text/text.ts
Outdated
* - inline-2: font-size: 14px; line-height: 16px; font-weight: 400; font-family: var(--yc-font-family-monospace); | ||
* - inline-3: font-size: 16px; line-height: 20px; font-weight: 400; font-family: var(--yc-font-family-monospace); | ||
*/ | ||
typography?: typeof TYPOGRAPHY_VARIANTS[number]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe call it variant
?
* - 1: font-size: 28px; line-height: 36px; font-weight: 900; | ||
* - 2: font-size: 32px; line-height: 40px; font-weight: 900; | ||
* - 3: font-size: 40px; line-height: 48px; font-weight: 900; | ||
* - 4: font-size: 48px; line-height: 52px; font-weight: 900; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
font-weight
is controlled by variables, and can be changed outside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main idea of this comments is ability to quick look at the component and understand what it does. Of course we can override every token
of text variant.
I will add note about it in this comment
src/components/Text/text/text.ts
Outdated
'code-inline-1', | ||
] as const; | ||
|
||
export interface TextBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a props them it should have Props
prefix
src/components/Text/text/text.ts
Outdated
|
||
const b = block('text'); | ||
|
||
export const TYPOGRAPHY_VARIANTS = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TEXT_VARIANTS
is better imo, it shows connection to Text
component like TEXT_COLORS
does.
@@ -0,0 +1,3 @@ | |||
export * from './Text'; | |||
export {TYPOGRAPHY_VARIANTS, text} from './text/text'; | |||
export {TEXT_COLORS, colorText} from './colorText/colorText'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's name "not a component or hook or class" files in dash-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
colorText
is a component ("block" in BEM paradigm). But yes it's not a "react" component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it's better the entity name and the file name match
@use '../../variables' as variables; | ||
@use '../../../../styles/mixins' as mixins; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default its already in these namespaces (ns = filename)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed as variables
and as mixins;
;
e5f5ae8
to
ba90b00
Compare
/fixed |
Hi, this is related to #128
what was done:
Text
component;text
andcolorText
utilities to work with inline classes in existing codebase;@mixin text-display-3
instyles/mixins.scss
;also added documentation to storybook