-
-
Notifications
You must be signed in to change notification settings - Fork 113
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: implement table component #444
Conversation
Pull Request Test Coverage Report for Build 684
💛 - Coveralls |
src/components/Table/index.js
Outdated
} | ||
|
||
Table.propTypes = { | ||
data: PropTypes.array.isRequired, |
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.
Add the descriptions to the props.
import Rows from './body'; | ||
import Headers from './head'; | ||
import './styles.css'; | ||
|
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.
Add the component description
src/components/Table/readme.md
Outdated
@@ -0,0 +1,40 @@ | |||
#Basic Table |
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 need change this title by ##### simple Table
you have to export the Table component in the root index. |
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 looks good, some minor changes & I don't see any test
src/components/Table/body/index.js
Outdated
|
||
export default function Rows(props) { | ||
const { data, columns } = props; | ||
if (data && columns) { |
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 data and column are arrays since you are going to do map
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.
Array.isArray(data) && Array.isArray(columns)
src/components/Table/index.js
Outdated
|
||
componentDidMount() { | ||
if (!this.columns) { | ||
this.getColumns(); |
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 are resolving the columns only one time when the componentDidMount, what happen if the customer change the columns dynamically after the component was mounted
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 if the columns is always generated from the column passed by the developer and the change of the column imply render of the table then we should put it in the state of the component
src/components/Table/readme.md
Outdated
<Table data={tableData} style={tableStyles}> | ||
<Column header="Name" field="name" /> | ||
<Column header="Company" field="company" /> | ||
<Column header="email" field="email" /> |
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.
we should add more examples that use the component
prop of the Column component
start the library and open the console when develop, there are lot of warnings, and one is very important: |
src/components/Column/index.js
Outdated
PropTypes.string, | ||
]), | ||
/** The component with wich to render the content of each cell */ | ||
component: PropTypes.node.isRequired, |
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 that component should not be required
Also add a simple table example for Column component
src/components/Table/index.js
Outdated
return ( | ||
<table className={className} style={style}> | ||
<thead className="rainbow-table_head"> | ||
<tr className="rainbow-table_header-row"> |
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.
fix indentation
src/components/Table/index.js
Outdated
</tr> | ||
</thead> | ||
<tbody className="rainbow-table_body"> | ||
<Rows data={data} columns={columns} /> |
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.
same ^^
src/components/Table/index.js
Outdated
|
||
Table.propTypes = { | ||
/** An array containing the objects to be displayed */ | ||
data: PropTypes.array.isRequired, |
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.
set prop as array of objects
fixes #289