-
Notifications
You must be signed in to change notification settings - Fork 2
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
Attempt to revise <TextLabel> (pt. 1) #5
Conversation
a8e6f97
to
6f86a22
Compare
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.
Others LGTM 👍
src/Text/BasicRow.js
Outdated
import Tag from '../Tag'; | ||
import TextEllipsis from '../TextEllipsis'; | ||
|
||
function wrapIfNotElement(content, { with: Wrapper }) { |
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.
Can add some simple description to describe how this method works?
Since I cannot understand this method straightly from it's name XD
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.
OK I'll add comments XD
But basically it's a function to check: if the passed-in content
is not a valid React Element, then wrap it with Wrapper
. Otherwise just return the content
.
6f86a22
to
aa10cda
Compare
@cjies I've added the comments as suggested, and I also added some tests to the new components introduced in this PR. Please help take a look again. Here's the diffs: 842e0c5...aa10cda |
Purpose
Try to simplify the underlying structure of
<TextLabel>
, starting with replacing<TextLayout>
with<Text>
.My current plan is:
Stop using
<TextLabel>
inside almost every component.Previous experiences have shown such usage results in a huge, complex mega component with too many features.
Compose every base component using
<Icon>
and<Text>
Components
➕
<Text>
(proposed to replace<TextLayout>
)➕
<BasicRow>
(stays underText/
, not exposed.)Usage
The row for basic text can be replace via
basicRow
prop.In some cases like
<TextInput>
it can replace the whole row with a general<input>
so it should be able to take up full-width.DOM output
Output of current implement of
<Text>
while the previous output of
<TextLayout>
in iC-framework-react:Dependency
#4