Skip to content
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

Allow user to render their own wrapper (useful for tables) #30

Closed
wants to merge 1 commit into from

Conversation

eyn
Copy link

@eyn eyn commented Nov 3, 2017

Thanks for the great library! I've added a renderWrapper function this allows a user to use components other the <div> for the two wrappers (such as table and tbody with appropriate styling).

An example using material-ui:

import * as React from 'react';
import * as ReactDOM from 'react-dom';
import { Table, TableHead, TableBody, TableCell, TableRow } from 'material-ui';
import { withStyles } from 'material-ui/styles';

import VirtualList from '../../src/';
import './demo.css';

const styles = ({
  table: {
    display: 'block',
    '& thead': {
      display: 'block',
    },
    '& tbody': {
      display: 'block',
    },
    '& tr': {
      display: 'table',
      width: '100%',
    },
  },
});

const TableWithRef = withStyles<any>(styles)(
  ({ classes, className, tableRef, children, ...others }: any) =>
    <table className={`${classes.table} ${className}`} ref={tableRef} {...others}>
      {children}
    </table>);

class Demo extends React.Component {
  renderWrapper = (outerProps, ref, innerStyle, items) => {
    return (
      <Table tableRef={ref} {...outerProps} component={TableWithRef}>
        <TableHead>
          <TableRow>
            <TableCell>Test</TableCell>
          </TableRow>
        </TableHead>
        <TableBody style={innerStyle}>
          {items}
        </TableBody>
      </Table>);
  }

  renderItem = ({ style, index }: { style: React.CSSProperties, index: number }) => {
    return (
      <TableRow className="Row" style={style} key={index}>
        <TableCell>
          Row #{index}
        </TableCell>
      </TableRow>
    );
  }

  render() {
    return (
      <div className="Root">
        <VirtualList
          width="auto"
          height={400}
          itemCount={1000}
          renderItem={this.renderItem}
          renderWrapper={this.renderWrapper}
          itemSize={58}
          className="VirtualList"
        />
      </div>
    );
  }
}

ReactDOM.render(<Demo />, document.querySelector('#app'));

@codecov
Copy link

codecov bot commented Nov 3, 2017

Codecov Report

Merging #30 into master will increase coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #30     +/-   ##
========================================
+ Coverage    93.6%   93.7%   +0.1%     
========================================
  Files           3       3             
  Lines         125     127      +2     
  Branches       14      14             
========================================
+ Hits          117     119      +2     
  Misses          8       8
Impacted Files Coverage Δ
src/index.tsx 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e97d432...3ed4012. Read the comment docs.

@eyn
Copy link
Author

eyn commented Dec 3, 2017

Hi @clauderic - any chance of getting some feedback/getting this merged? Thanks for the awesome library :)

@thorjarhun
Copy link

Hi @eyn. I really like the functionality added in this PR! Thank you so much for sharing!

I'd like to use the wrapper from your example using material-ui but I'm using the material-ui v1 which doesn't have support for the old fixedHeader prop. I'm also using the scrollToTransition proposal (open at time of writing) but found that I needed to modify the getOffsetForIndex method to add the height of the header to the value returned. I would love a fixed header and I think some solutions to achieve that will resolve my problem with getOffsetForIndex. I've done some research and it appears that there are many solutions for achieving a fixed-header table but I haven't yet found one that works. By chance, are you using a material-ui table with a fixed header or have advice on what I should try to achieve it? Perhaps we could even add your example as a suggestion for using renderWrapper in the docs, assuming this PR goes through!

@eyn
Copy link
Author

eyn commented Jan 4, 2018

Hi @thorjarhun

Glad you found the PR useful! I've also struggled with getting a fixed header and a better solution is on my to-do list. At the moment I've gone down the route of using two tables - one for the header and one for the body with fixed column % widths as I know the length of most of the data I'm displaying.

Would that work for you?

@thorjarhun
Copy link

That would be fantastic! Is the code hosted somewhere for reference? I couldn't find anything that looks like a to-do list in your repo's.

@eyn
Copy link
Author

eyn commented Jan 4, 2018

Ahh I don't have any public code for that - I've just copied the bit out of the relevant file below:

          <div className={classes.tableContainer}>
            <Table className={classes.headerTable}>
              <TableHead>
                <TableRow>
                  <TableCell
                    className={classes.checkboxCell}
                    padding="checkbox"
                  >
                    <Checkbox
                      indeterminate={
                        select.selectedRowCount > 0 &&
                        select.selectedRowCount < rows.length
                      }
                      checked={select.selectedRowCount === rows.length}
                      onClick={this.handleSelectAllClick}
                    />
                  </TableCell>
                  {columns.map(col => (
                    <TableHeaderCell
                      key={col.name}
                      col={col}
                      sort={sort}
                      showFilter={showFilterList || col.filter.isFiltered}
                    />
                  ))}
                  <TableCell padding="none" className={classes.cellToolbar} />
                </TableRow>
              </TableHead>
            </Table>
            <VirtualList
              width="auto"
              height={height}
              itemCount={rows.length}
              renderItem={this.renderItem}
              renderWrapper={this.renderWrapper}
              tableState={tableState}
              itemSize={48}
              overscanCount={20}
              className={classes.bodyTable}
              rows={rows}
            />
          </div>

Hopefully that will get you pointed in the right direction

@mihiramin89
Copy link

Any updates on when this will be merged? I am using a table that can really use this for the body.

@prescottprue
Copy link

I agree this would be useful. Any input on when it is getting merged?

@vste23
Copy link

vste23 commented May 9, 2018

Hi. Very useful PR that would be awesome to see merged into this library. Will it be merged?

@towfiqi
Copy link

towfiqi commented Aug 4, 2018

+1 Kindly merge this..

@atrauzzi
Copy link

Yikes, what's the holdup here?

@baronmog
Copy link

Is this going anywhere? I'm working on a project that already uses react-tiny-virtual-list. We're in the process of integrating material-ui into it and would like to continue using react-tiny-virtual-list, but...

@eyn
Copy link
Author

eyn commented Aug 22, 2018

I've tried contacting @clauderic via other channels but no success so doesn't look like it 😢 I have been using this patch in production for quite a while so it does work..

@clauderic
Copy link
Owner

This PR has merge conflicts and I'm not 100% satisfied with the approach, the function signature for the renderWrapper is too complex in my opinion.

I'm going to close this PR for the moment as I don't have any intention of merging it in it's current state, but we can keep the discussion open.

To be clear, I agree that this is an important use case and I am willing to support it, but I'd rather spend a bit more time to make the API more elegant.

@clauderic clauderic closed this Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants