-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[Doc] Slightly improved propsTable layout #2425 #2426
Conversation
@frankf-cgn This looks good. thank you. Only one issue, please make deafults more contrast, it's a bit hard to read |
@@ -51,7 +53,9 @@ const PropTypeDescription = React.createClass({ | |||
} | |||
|
|||
return ( | |||
<MarkdownElement text={text} /> | |||
<div className={'propsTable'}> |
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.
Wouldn't className="propsTable"
be better?
Awesome 👍 👍 Please do a rebase and we'll merge this. |
@oliviertassinari Let me rebase if you are ok with the code. |
@@ -51,7 +53,9 @@ const PropTypeDescription = React.createClass({ | |||
} | |||
|
|||
return ( | |||
<MarkdownElement text={text} /> | |||
<div className="propsTable"> |
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'm gonna be rude, but I think that PropTypeDescription
or in lower case would be a more explicit className. Though ?
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'm gonna do it, but I think that way too verbose. propTypeTable
maybe? But one is good as another, I think ;-)
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 propsTable
is the balance between verbosity and explicitness. 👍 for propsTable
😁
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.
Then we should rename the component to PropsTable
?
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 don't think so, let me rename the className, ok?
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.
@oliviertassinari 😱 I see your point. PropTypeDescription
👍
2ee5a6b
to
3423184
Compare
Fixed the classname and rebased. |
@frankf-cgn Thanks. Should we rename |
3423184
to
bbc0d30
Compare
@oliviertassinari You are right. Done. |
@frankf-cgn Awesome, thanks! |
[Doc] Slightly improved propsTable layout #2425
@frankf-cgn The props look awesome thanks a bunch 👍 🎉 |
Because styling a table in markdown is not directly supported, I have reverted to a pure css solution and wrapped the
<MarkdownElement>
in<PropTypeDescription>
with a div.Fix #2425