Reduce dom levels in the default cell renderer #556
Conversation
There was a problem hiding this comment.
I don't think the renderer changes shown in this review can be deemed as 'non-breaking.'
The new renderer has slightly different css, but the biggest change would be the change in nesting of elements (or lack of).
@wcjordan , how about we mark this cell renderer as deprecated for now? We could then provide the newer cell renderer as part of the API so that users can gradually migrate to it.
| @@ -87,26 +87,14 @@ class FixedDataTableCellDefault extends React.Component { | |||
| <div | |||
| {...props} | |||
| className={joinClasses( | |||
| cx('fixedDataTableCellLayout/wrap1'), | |||
| cx('public/fixedDataTableCell/wrap1'), | |||
| cx('fixedDataTableCellLayout/wrap'), | |||
There was a problem hiding this comment.
We condense all the 3 wrapper levels into the top level div.
Also I'm pulling up the cellContent class name.
| @@ -29,10 +29,9 @@ | |||
| background-color: #0284ff; | |||
| } | |||
| .public/fixedDataTableCell/hasReorderHandle .public/fixedDataTableCell/cellContent { | |||
| margin-left: 12px; | |||
| padding-left: 20px; | |||
There was a problem hiding this comment.
Margin was added here so as to leave some space if the reorder handle exists.
But now, the only wrapper element has display set to table-cell, and such elements are unable to have margin.
So we'll just add these to padding instead. It should have the same effect for most use cases.
But if the user wants to add margin, he can either
- change the display property
- change padding
- just make a new wrapper element
pradeepnschrodinger
requested review from
somavara,
wcjordan,
mycrobe and
miskreant
|
I like that idea. Let's deprecate these and remove in v2. We can provide the new one and switch all the examples over to use the new one. |
| @@ -0,0 +1,118 @@ | |||
| /** | |||
There was a problem hiding this comment.
This file is just the old FixedDataTableCellDefault.
There's no change besides a comment explaining this component is deprecated.
| * NOTE (pradeep): This component is deprecated since it uses a lot of wrapper DIV nodes for styling/layout. | ||
| * The replacement is src/FixedDataTableCell.js which uses a single wrapper to achieve the same table cell layout. |
There was a problem hiding this comment.
The only change in the file.
src/FixedDataTableRoot.js
Outdated
| import Column from 'FixedDataTableColumn'; | ||
| import ColumnGroup from 'FixedDataTableColumnGroup'; | ||
|
|
||
| const version = '1.1.0'; | ||
|
|
||
| export { | ||
| Cell, | ||
| CellDeprecated, |
There was a problem hiding this comment.
Users who still wanna use the old cell will need to export CellDeprecated
There was a problem hiding this comment.
This would still be a breaking change in terms of API right. How about naming the new cell something like DataCell to avoid name conflict? CC: @wcjordan
There was a problem hiding this comment.
I'm all for not breaking the API until a major update. DataCell sounds fine to me.
|
As discussed above, to not break the API, we'll rename the newer cell component to @somavara, @wcjordan , could you take a look again and give another sign off? |
Introduced new cell renderer `DataCell`, which is a leaner implementation of the default `Cell` renderer.
|
Merged to master. |
Description
Minimize dom components in the default cell renderer, as part of #396.
See LD side changes required at SB-18854
Motivation and Context
Increase performance by reducing 3 dom levels per cell.
How Has This Been Tested?
Tested on our examples and also on LD.
Types of changes
Checklist: