-
Notifications
You must be signed in to change notification settings - Fork 380
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(react-grid): implement row templates #333
feat(react-grid): implement row templates #333
Conversation
)} | ||
tableStubCellTemplate={stubCellTemplate} | ||
tableStubHeaderCellTemplate={stubHeaderCellTemplate} | ||
tableNoDataCellTemplate={noDataCellTemplate} |
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 don't we allow to customize three templates above?
> | ||
{children} | ||
</tr> | ||
export const TableRow = ({ tableRow, children, ...restProps }) => ( |
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 we should clearly define interface here like we do in table-cell.jsx.
children, | ||
changeSelected, | ||
selectByRowClick, | ||
...restProps |
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 we should clearly define interface here like we do in table-cell.jsx.
> | ||
{children} | ||
</TableRowMUI> | ||
export const TableRow = ({ tableRow, children, ...restProps }) => ( |
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 we should clearly define interface here like we do in table-cell.jsx.
children, | ||
changeSelected, | ||
selectByRowClick, | ||
...restProps |
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 we should clearly define interface here like we do in table-cell.jsx.
...restProps | ||
}) => ( | ||
<TableRow | ||
selected={tableRow.selected} |
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.
Seems like selected prop should on the root options layer rather than inside tableRow.
...restProps | ||
}) => ( | ||
<tr | ||
className={tableRow.selected ? 'active' : ''} |
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.
Seems like selected prop should on the root options layer rather than inside tableRow.
}) => ( | ||
<tr | ||
className={tableRow.selected ? 'active' : ''} | ||
onClick={selectByRowClick ? (e) => { |
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 when this option is not enabled, we should avoid rendering that template. It may unexpectedly override custom user template. So, we should add a note to the docs that rowTemplate for data rows defined in TableView, will be overriden by selectRowTemplate when the property is true.
<tr | ||
className={tableRow.selected ? 'active' : ''} | ||
{...restProps} | ||
style={{ |
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.
Change to stype={style}
e.stopPropagation(); | ||
changeSelected(); | ||
} : undefined | ||
} |
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 change this to:
(e) => {
if (!selectByRowClick) return;
e.stopPropagation();
changeSelected();
}
It reads naturally.
<TableRowMUI | ||
selected={tableRow.selected} | ||
{...restProps} | ||
style={{ |
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 same here
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
|
||
export const TableSelectRow = ({ |
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 don't this template pass styles?
import PropTypes from 'prop-types'; | ||
import { TableRow } from 'material-ui'; | ||
|
||
export const TableSelectRow = ({ |
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 here
<TableRow | ||
selected={selected} | ||
onClick={ | ||
selectByRowClick ? (e) => { |
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 here
@@ -13,8 +13,8 @@ This plugin visualizes the selection state within a table by rendering selection | |||
|
|||
Name | Type | Default | Description | |||
-----|------|---------|------------ | |||
highlightSelected | boolean | false | If true, selected rows are highlighted | |||
selectByRowClick | boolean | false | If true, a selected row is toggled by click | |||
highlightSelected | boolean | false | If true, selected rows are highlighted. Pay attention, the `tableRowTemplate` of the `TableView` plugin will be ignored in this 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.
Ok, it will be ignored. And how to customise a template with enabled option?
------|------|------------ | ||
tableRow | [TableRow](#table-row) | Specifies a table row | ||
children | ReactElement | A React element used to render a table row. | ||
style? | Object | Styles that should be applied to the root row element |
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.
What about the row argument? I think it is much more intuitive to use a user-defined row instead of accessing it from the row field of the tableRow interface.
@@ -15,6 +15,7 @@ Name | Type | Default | Description | |||
-----|------|---------|------------ | |||
rowHeight | number | | Specifies the edit row height | |||
editCellTemplate | (args: [EditCellArgs](#edit-cell-args)) => ReactElement | | A component that renders an editable cell | |||
editRowTemplate | (args: [TableRowArgs](table-view.md#table-row-args)) => ReactElement | | A component that renders an editable 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.
Shouldn't we stress here and in other similar places that it renders row container not the whole row with content?
BREAKING CHANGE:
The
tableExtraProps
getter was removed from theTableView
andTableSelection
plugins.