-
Notifications
You must be signed in to change notification settings - Fork 2
feat: add Text component #2408
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
base: main
Are you sure you want to change the base?
feat: add Text component #2408
Conversation
🔍 Visual review for your branch is published 🔍Here are the links to: |
import { textVariants } from "./variants" | ||
|
||
export type AsAllowedList = | ||
| "div" |
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.
Why would we want to have a text as a div? 🤔
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've seen it used a lot for generic layouts, but it's probably bad for accessibility, given that we have plenty of other tags that suit text better. I can remove it if we feel it doesn't fit there 👍
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.
div
is a way to render the text as block
element.
Maybe we can remove span and div and use the prop block
or inline
to render them block or inline
import { textVariants } from "./variants" | ||
|
||
export type AsAllowedList = | ||
| "div" |
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.
div
is a way to render the text as block
element.
Maybe we can remove span and div and use the prop block
or inline
to render them block or inline
| "p" | ||
| "label" | ||
| "span" | ||
| "h1" |
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 wondering If makes sense to create an specific component F0Heading
for the headings, its a way to control what is a heading or not, visually can be similar to text, but semantic are diferent
Coverage Report for packages/react
File Coverage
|
size-limit report 📦
|
Description
Text is a typography component that handles styling of different semantic usages of texts across Factorial. It features:
heading
, it will render ah2
, but if you usebody
, it will render ap
. Optionally, the user can choose a different element to render, through theas
prop.Screenshots
Implementation details
className
) and private variants, only used inside the design system, but not in Factorial.OneEllipsis
(component used for the ellipsis) andF0Text
, there's a prop to choose in which HTML element is the text rendered. I've added that option toOneEllipsis
.