facebookarchive / fixed-data-table

A React table component designed to allow presenting thousands of rows of data.
http://facebook.github.io/fixed-data-table/
Other
4.3k stars 553 forks source link

Inefficient DOM structure (unnecessary nested markup) #263

Open bvaughn opened 9 years ago

bvaughn commented 9 years ago

Hi guys. Thanks for creating and maintaining this library. :)

I'm working on a project that uses it. I recent took a look at the markup generated this component and it does not seem very optimized. Each "cell" in the "table" (what would be a <td> in a regular HTML <table>) actually consists of many nested nodes:

<!-- display: inline-block -->
<div class="fixedDataTableCellLayout_main public_fixedDataTableCell_main">
  <!-- display: table -->
  <div class="fixedDataTableCellLayout_wrap1 public_fixedDataTableCell_wrap1">
    <!-- display: table-row -->
    <div class="fixedDataTableCellLayout_wrap2 public_fixedDataTableCell_wrap2">
      <!-- display: table-cell -->
      <div class="fixedDataTableCellLayout_wrap3 public_fixedDataTableCell_wrap3">
        <!-- display: block -->
        <div class="public_fixedDataTableCell_cellContent">
          <!-- Cell content -->
        </div>
      </div>
    </div>
  </div>
</div>

I understand that plain HTML <table> doesn't work well for the fixed header interface this library is offering. But why is this nested structure required for each cell? So far as I know, nesting a table inside of an inline-block element doesn't do anything more than just using the inline-block element directly.

Why could the above markup not be simplified to:

<!-- display: inline-block -->
<div class="fixedDataTableCellLayout_main public_fixedDataTableCell_main">
  <!-- display: block -->
  <div class="public_fixedDataTableCell_cellContent">
    <!-- Cell content -->
  </div>
</div>

For that matter, is the inner cell-content wrapper even necessary? Why not just:

<!-- display: inline-block -->
<div class="fixedDataTableCellLayout_main public_fixedDataTableCell_main">
  <!-- Cell content -->
</div>

At the point where you're managing widths and row-heights to make things appear like a classical HTML table, I don't see what the display: table rules buy you. Any chance you could collapse some of these nodes and lighten the DOM load on the browser?

ehzhang commented 9 years ago

These nested nodes are currently necessary for the method of vertical alignment for each cell. We're currently working on a release that does flatten this though.

bvaughn commented 9 years ago

I see. I'm surprised to hear that's the reason. But I'm glad to hear it will be addressed with an update. Thank you for the information. :)

rmilesson commented 9 years ago

+1

Vertical alignment is meh, the users can handle that themselves since we're all in control over row height anyway.

jamesseanwright commented 8 years ago

To dig this up from last year, I definitely would not use this unless you moved to table elements over divs. table is semantic, whereas div isn't. Are there any plans to do this?

bvaughn commented 8 years ago

@jamesseanwright Check out react-virtualized FlexTable as a possible alternative if you're looking for similar functionality with a cleaner DOM.

(It also uses <div> but with aria roles.)