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.

Sign up for GitHub

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

Jump to bottom

Do not crash when unsetting scrollToRow #62

Merged
merged 4 commits into from
Oct 11, 2016
Merged

Do not crash when unsetting scrollToRow #62

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
634de7d
Do not crash when unsetting scrollToRow
IanVS Oct 10, 2016
c7f5330
Adding semicolons
Oct 11, 2016
d6be9c8
Adding more tests and fixing for scrollTop
Oct 11, 2016
4e36c9c
CR feedback
Oct 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions build_helpers/test-globals.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
'use strict'

var jsdom = require('jsdom')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing semicolons throughout this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I get for copying it from somewhere else. Sure would be nice to have ESLint set up for this project. Any interest in a PR doing so?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, would love it!


// setup the simplest document possible
var doc = jsdom.jsdom('<!doctype html><html><body></body></html>')

// get the window object out of the document
var win = doc.defaultView

// set globals for mocha that make access to document and window feel
// natural in the test environment
global.document = doc
global.window = win

// take all properties of the window object and also attach it to the
// mocha global object
propagateToGlobal(win)

// from mocha-jsdom https://github.com/rstacruz/mocha-jsdom/blob/master/index.js#L80
function propagateToGlobal (window) {
for (let key in window) {
if (!window.hasOwnProperty(key)) continue
if (key in global) continue

global[key] = window[key]
}
}
6 changes: 4 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"file-loader": "^0.8.1",
"glob": "^4.3.5",
"html-loader": "^0.2.3",
"jsdom": "^9.6.0",
"less": "^2.2.0",
"less-loader": "^2.0.0",
"marked": "^0.3.2",
Expand All @@ -37,6 +38,7 @@
"react": "^15.2.1",
"react-addons-test-utils": "^15.2.1",
"react-docgen": "^1.2.0",
"react-dom": "^15.3.2",
"react-tools": "^0.12.2",
"sinon": "^2.0.0-pre.2",
"style-loader": "^0.8.3",
Expand All @@ -52,8 +54,8 @@
"build-docs": "./build_helpers/buildAPIDocs.sh",
"publish-site": "./build_helpers/publishStaticSite.sh",
"publish-package": "./build_helpers/publishPackage.sh",
"test": "mocha-webpack --webpack-config webpack.config-test.js \"src/**/*-test.js\"",
"test:watch": "mocha-webpack --webpack-config webpack.config-test.js --watch \"src/**/*-test.js\"",
"test": "mocha-webpack --webpack-config webpack.config-test.js \"src/**/*-test.js\" --require build_helpers/test-globals.js",
"test:watch": "mocha-webpack --webpack-config webpack.config-test.js --watch \"src/**/*-test.js\" --require build_helpers/test-globals.js",
"test:server": "webpack-dev-server --config webpack.config-test.js --hot --inline"
},
"repository": {
Expand Down
2 changes: 1 addition & 1 deletion src/FixedDataTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,7 @@ var FixedDataTable = React.createClass({
}

var lastScrollToRow = oldState ? oldState.scrollToRow : undefined;
if (props.scrollToRow !== lastScrollToRow) {
if (typeof props.scrollToRow !== 'undefined' && props.scrollToRow !== null && props.scrollToRow !== lastScrollToRow) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should introduce lodash into our lib and use _.isNil instead?
We wouldn't need to worry about bloat if we implemented tree shaking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be cool, but I guess I would argue that could be a separate PR after this one (if this is accepted).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to implement something similar for the other: scrollTop, scrollToRow and scrollToColumn

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought so too, but they weren't failing similar tests, while this one was, so I opted not to.

scrollState = this._scrollHelper.scrollRowIntoView(props.scrollToRow);
firstRowIndex = scrollState.index;
firstRowOffset = scrollState.offset;
Expand Down
53 changes: 50 additions & 3 deletions src/FixedDataTableRoot-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import { assert } from 'chai';
import React from 'react';
import FixedDataTable from './FixedDataTableRoot';
import { createRenderer, isElement } from 'react-addons-test-utils';
import { createRenderer, isElement, renderIntoDocument, findRenderedComponentWithType } from 'react-addons-test-utils';

const { Table, Column } = FixedDataTable;

Expand Down Expand Up @@ -77,7 +77,54 @@ describe('FixedDataTableRoot', function() {
assert.isBelow(table.state.scrollY, 30 * 100, 'should be below first row');
assert.isAbove(table.state.scrollY, 30 * 100 - 300, 'should be above last row');
});

});
});

describe('update render', function() {
let renderedTable;
beforeEach(function() {
class TableHOC extends React.Component {
constructor(props) {
super(props);
this.state = {
scrollToRow: 0,
};
}

render() {
return (
<Table
scrollToRow={this.state.scrollToRow}
{...this.props}
>
{this.props.children}
</Table>
);
}
}

const table = (
<TableHOC
width={600}
height={400}
rowsCount={50}
rowHeight={100}
headerHeight={50}
>
<Column width={300} />
<Column width={300} />
<Column width={300} />
<Column width={300} />
<Column width={300} />
</TableHOC>
);
const renderedTree = renderIntoDocument(table);
renderedTable = findRenderedComponentWithType(renderedTree, TableHOC)
})

it('should not blow up when unsetting the scrollToRow property', function() {
assert.doesNotThrow(function() {
renderedTable.setState({scrollToRow: undefined});
})
});
})
});