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

SS-30102: Add react-lifecycles-compat polyfill to FDT to support v1.1… #523

Merged
merged 3 commits into from
Apr 7, 2020
Merged

SS-30102: Add react-lifecycles-compat polyfill to FDT to support v1.1… #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f80bcf
SS-30102: Add react-lifecycles-compat polyfill to FDT to support v1.1…
pradeepnschrodinger Mar 31, 2020
7466cf7
change back usage of componentDidUpdate to componentWillReceiveProps
pradeepnschrodinger Apr 7, 2020
ca60fd9
use old name for `componentWillReceiveProps`
pradeepnschrodinger Apr 7, 2020
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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"dependencies": {
"lodash": "^4.17.4",
"prop-types": "^15.7.2",
"react-lifecycles-compat": "^3.0.4",
"redux": "^4.0.1",
"reselect": "^4.0.0"
},
Expand Down
4 changes: 1 addition & 3 deletions src/FixedDataTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,7 @@ class FixedDataTable extends React.Component {
}

componentDidUpdate(/*object*/ prevProps) {
if (prevProps !== this.props) {
Copy link
Member

Choose a reason for hiding this comment

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

no need to check for change anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, because the this._didScroll already compares the scroll props before dispatching actions.

Furthermore, I believe comparing just the prop object's reference in order to skip actions might lead to bugs in the future (like if the user just changed the classname prop but the action still fires because prop reference will get changed). So it's better if the action itself just selects and compares the props it needs.

this._didScroll(prevProps);
}
this._didScroll(prevProps);
this._reportContentHeight();
this._reportScrollBarsUpdates();
}
Expand Down
121 changes: 60 additions & 61 deletions src/FixedDataTableCell.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import PropTypes from 'prop-types';
import cx from 'cx';
import joinClasses from 'joinClasses';
import shallowEqual from 'shallowEqual';
import { polyfill as lifecycleCompatibilityPolyfill } from 'react-lifecycles-compat';

class FixedDataTableCell extends React.Component {
/**
Expand Down Expand Up @@ -125,79 +126,76 @@ class FixedDataTableCell extends React.Component {
return false;
}

componentDidUpdate(prevProps) {
if (prevProps === this.props) {
return;
}

const props = this.props;
var left = props.left + this.state.displacement;
static getDerivedStateFromProps(nextProps, prevState) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's no net change in the function, I just reordered statements to make it cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

so is it just changing componentDidUpdate to getDerivedStateFromProps + reordering?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. This ensures a single render after state update in response to prop change.

Copy link
Member

Choose a reason for hiding this comment

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

cool. But to ensure I understand, we still get 2 rerenders, one for prop changes to the top level component, and another after the redux action is done?

var left = nextProps.left + prevState.displacement;

var newState = {
isReorderingThisColumn: false
isReorderingThisColumn: false,
};

if (props.isColumnReordering) {
var originalLeft = props.columnReorderingData.originalLeft;
var reorderCellLeft = originalLeft + props.columnReorderingData.dragDistance;
var farthestPossiblePoint = props.columnGroupWidth - props.columnReorderingData.columnWidth;
if (!nextProps.isColumnReordering) {
newState.displacement = 0;
return newState;
}

var originalLeft = nextProps.columnReorderingData.originalLeft;
var reorderCellLeft = originalLeft + nextProps.columnReorderingData.dragDistance;
var farthestPossiblePoint = nextProps.columnGroupWidth - nextProps.columnReorderingData.columnWidth;

// ensure the cell isn't being dragged out of the column group
reorderCellLeft = Math.max(reorderCellLeft, 0);
reorderCellLeft = Math.min(reorderCellLeft, farthestPossiblePoint);

// check if current cell belongs to the column that's being reordered
if (nextProps.columnKey === nextProps.columnReorderingData.columnKey) {
newState.displacement = reorderCellLeft - nextProps.left;
newState.isReorderingThisColumn = true;
return newState;
}

// ensure the cell isn't being dragged out of the column group
reorderCellLeft = Math.max(reorderCellLeft, 0);
reorderCellLeft = Math.min(reorderCellLeft, farthestPossiblePoint);
var reorderCellRight = reorderCellLeft + nextProps.columnReorderingData.columnWidth;
var reorderCellCenter = reorderCellLeft + (nextProps.columnReorderingData.columnWidth / 2);
var centerOfThisColumn = left + (nextProps.width / 2);

if (props.columnKey === props.columnReorderingData.columnKey) {
newState.displacement = reorderCellLeft - props.left;
newState.isReorderingThisColumn = true;
} else {
var reorderCellRight = reorderCellLeft + props.columnReorderingData.columnWidth;
var reorderCellCenter = reorderCellLeft + (props.columnReorderingData.columnWidth / 2);
var centerOfThisColumn = left + (props.width / 2);

var cellIsBeforeOneBeingDragged = reorderCellCenter > centerOfThisColumn;
var cellWasOriginallyBeforeOneBeingDragged = originalLeft > props.left;
var changedPosition = false;

if (cellIsBeforeOneBeingDragged) {
if (reorderCellLeft < centerOfThisColumn) {
changedPosition = true;
if (cellWasOriginallyBeforeOneBeingDragged) {
newState.displacement = props.columnReorderingData.columnWidth;
} else {
newState.displacement = 0;
}
}
var cellIsBeforeOneBeingDragged = reorderCellCenter > centerOfThisColumn;
var cellWasOriginallyBeforeOneBeingDragged = originalLeft > nextProps.left;
var changedPosition = false;

if (cellIsBeforeOneBeingDragged) {
if (reorderCellLeft < centerOfThisColumn) {
changedPosition = true;
if (cellWasOriginallyBeforeOneBeingDragged) {
newState.displacement = nextProps.columnReorderingData.columnWidth;
} else {
if (reorderCellRight > centerOfThisColumn) {
changedPosition = true;
if (cellWasOriginallyBeforeOneBeingDragged) {
newState.displacement = 0;
} else {
newState.displacement = props.columnReorderingData.columnWidth * -1;
}
}
newState.displacement = 0;
}

if (changedPosition) {
if (cellIsBeforeOneBeingDragged) {
if (!props.columnReorderingData.columnAfter) {
props.columnReorderingData.columnAfter = props.columnKey;
}
} else {
props.columnReorderingData.columnBefore = props.columnKey;
}
} else if (cellIsBeforeOneBeingDragged) {
props.columnReorderingData.columnBefore = props.columnKey;
} else if (!props.columnReorderingData.columnAfter) {
props.columnReorderingData.columnAfter = props.columnKey;
}
} else {
if (reorderCellRight > centerOfThisColumn) {
changedPosition = true;
if (cellWasOriginallyBeforeOneBeingDragged) {
newState.displacement = 0;
} else {
newState.displacement = nextProps.columnReorderingData.columnWidth * -1;
}
}
}

if (changedPosition) {
if (cellIsBeforeOneBeingDragged) {
if (!nextProps.columnReorderingData.columnAfter) {
nextProps.columnReorderingData.columnAfter = nextProps.columnKey;
}
} else {
nextProps.columnReorderingData.columnBefore = nextProps.columnKey;
}
} else {
newState.displacement = 0;
} else if (cellIsBeforeOneBeingDragged) {
nextProps.columnReorderingData.columnBefore = nextProps.columnKey;
} else if (!nextProps.columnReorderingData.columnAfter) {
nextProps.columnReorderingData.columnAfter = nextProps.columnKey;
}

this.setState(newState);
return newState;
}

static defaultProps = /*object*/ {
Expand Down Expand Up @@ -347,4 +345,5 @@ class FixedDataTableCell extends React.Component {
event.stopPropagation();
}
}
export default FixedDataTableCell;

export default lifecycleCompatibilityPolyfill(FixedDataTableCell);
15 changes: 5 additions & 10 deletions src/FixedDataTableContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,16 @@ class FixedDataTableContainer extends React.Component {
this.state = this.getBoundState();
}

componentDidUpdate(prevProps) {
// Only react to prop changes - ignore updates due to internal state
if (this.props === prevProps) {
return;
}

componentWillReceiveProps(nextProps) {
invariant(
this.props.height !== undefined || this.props.maxHeight !== undefined,
nextProps.height !== undefined || nextProps.maxHeight !== undefined,
'You must set either a height or a maxHeight'
);

this.reduxStore.dispatch({
type: ActionTypes.PROP_CHANGE,
newProps: this.props,
oldProps: prevProps,
newProps: nextProps,
oldProps: this.props,
});
}

Expand All @@ -78,8 +73,8 @@ class FixedDataTableContainer extends React.Component {
render() {
const fdt = (
<FixedDataTable
{...this.state}
{...this.props}
{...this.state}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yet another bug, the order was changed in #509, and lead to other issues.

scrollActions={this.scrollActions}
columnActions={this.columnActions}
/>
Expand Down
Loading