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

Remove setScrollLeft on Row and Cell #1793

Merged
merged 2 commits into from
Oct 16, 2019
Merged

Remove setScrollLeft on Row and Cell #1793

merged 2 commits into from
Oct 16, 2019

Conversation

nstepien
Copy link
Contributor

  • It was only used in IE11
  • We now only pass the scrollLeft prop in IE11, as it's not used in other browsers.
    • Vertical scrolling isn't impacted in any way.
  • memo/SCU/PureComponent simplification:
    • we don't depend on shallowequal anymore
    • React's internal props comparison might be more optimized than shallowequal
  • Fewer refs, we'll be able to move Cell to a stateless functional component.
  • Performance isn't that much worse in IE11?

@nstepien nstepien self-assigned this Oct 16, 2019
@nstepien nstepien marked this pull request as ready for review October 16, 2019 18:23
@nstepien nstepien requested a review from qili26 October 16, 2019 18:23
ref={this.copyMask}
/>
)}
{copiedPosition && <CopyMask {...this.getSelectedDimensions(copiedPosition)} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the CopyMask? Like ctrl c / ctrl v background?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
A mask that appears when you copy a cell.

return <Renderer key={`${key as keyof R}-${idx}`} {...cellProps} />; // FIXME: fix key type
}
// FIXME: do we need this, considering columnsMetrics should have these columns sorted already?
const frozenColumns = columns.slice(0, this.props.lastFrozenColumnIndex + 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember I tried in my local that to put frozenColumns first and then nonFrozenColumns later. The row cells displaying was screwed up. Not sure why that happened as I didn't take time to investigate.

@qili26
Copy link
Contributor

qili26 commented Oct 16, 2019

Just want to add one more comment that the difference between the existing code and the new code is just the SCU on CellContent and children component. The performance would be improved after we reach the Cell refactoring.

The bottleneck in IE 11 is after horizontal scrolling. There were bunch of appendChild(), removeChild(), insertBefore() happening for react mount unmount. IE 11 just cannot handle those large amount of actions....

@nstepien nstepien merged commit 6a10450 into canary Oct 16, 2019
@nstepien nstepien deleted the noscroll branch October 16, 2019 20:16
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.

None yet

2 participants