Remove Mixins #1078

amanmahajan7 · 2018-01-11T18:28:26Z

Description

A few sentences describing the overall goals of the pull request's commits.

Migration strategies:
https://reactjs.org/blog/2016/07/13/mixins-considered-harmful.html#migrating-from-mixins

Use utility functions when possible
If there is just one component using mixin then embed the mixin logic right into the component
Higher Order Components are tricky to use as refs are used in many places
Remove mixins
- ScrollShim
- GridScrollMixin
- ColumnUtilsMixin
- ViewportScroll
- KeyboardHandleMixin
- DOMMetrics
  - MetricsMixin
  - MetricsComputatorMixin

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/adazzle/react-data-grid/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)

What is the new behavior?

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[ ] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

This reverts commit e46a22d.

This reverts commit bfd939d.

This reverts commit 1858fd0.

This reverts commit e8ce7da.

This reverts commit 320b843.

This reverts commit 4c5d43d.

This reverts commit 87521bb.

(ReactDataGrid component adds the MetricsComputatorMixin so mixin methods are available to any child component via the context api, it does not need to be added to the Grid component)

amanmahajan7 · 2018-01-16T22:48:21Z

packages/react-data-grid/src/HeaderRow.js

    onHeaderDrop: PropTypes.func
  },

-  mixins: [ColumnUtilsMixin],


Does not need to add utility functions as a mixin. They can be used directly

amanmahajan7 · 2018-01-17T01:54:27Z

packages/react-data-grid/src/Grid.js

    overScan: PropTypes.object
  },

-  mixins: [


GridScrollMixin is only used by the Grid component so moved the logic into the Grid component

DOMMetrics.MetricsComputatorMixin defines childContextTypes/getChildContext to implicitly pass metricsComputator through the component tree. ReactDataGrid (Parent) component is using the same mixin and acts as the context provider so Grid component does not need to add it. It can be safely removed

https://reactjs.org/docs/context.html

amanmahajan7 · 2018-01-17T02:34:16Z

packages/react-data-grid/src/ReactDataGrid.js

 const ReactDataGrid = createReactClass({
  displayName: 'ReactDataGrid',

-  mixins: [


ColumnMetricsMixin is only used by the ReactDataGrid component so moved the logic into the component

DOMMetrics.MetricsComputatorMixin and DOMMetrics.MetricsMixin mixins are used to create a parent child coupling though the unstable context API. The idea is to update children components on window.resize event. The parent component ReactDataGrid maintains a list of components and computator functions and calls metricsUpdated method on each component when the computator function returns a different value. Only two components (ReactDataGrid and Viewport via ViewportScrollMixin) were using this logic and it has been completely removed. The computator logic has been moved to individual components (check resize event handlers)

Some of the KeyboardHandlerMixin functions are moved to keyboardUtils and the remaining functions are moved into the component

amanmahajan7 · 2018-01-17T15:18:29Z

packages/react-data-grid/src/Viewport.js


 const Viewport = createReactClass({
  displayName: 'Viewport',
-  mixins: [ViewportScroll],


ViewportScroll helper functions are moved to viewportUtils and the remaining methods are copied into the Viewport component

chrisirhc · 2018-01-20T21:35:43Z

Nice, a great step towards modernizing this towards React 16 and beyond. This should resolve the remaining requirements for moving out of createReactClass.

I'm not sure what tests would be needed for this change though, since all existing behavior should remain the same.

malonecj

The strategy here looks good - I am keen to get this in. Perhaps we can add some tests around the utility functions that we migrated. Apart from that I think we are good to go

amanmahajan7 · 2018-01-22T20:24:25Z

@malonecj added unit tests for the utility functions

malonecj · 2018-01-23T10:14:06Z

packages/react-data-grid/src/utils/__tests__/scrollUtils.spec.js

+    expect(shim.style.width).toBe('5px');
+    expect(shim.style.height).toBe('10px');
+    expect(shim.className.indexOf('react-grid-ScrollShim')).not.toBe(-1);
+  });


…class instead of strict equality

malonecj · 2018-01-25T17:53:34Z

Awesome work on this one @amanmahajan7. One step closer to React 16

amanmahajan7 added 27 commits January 11, 2018 12:21

Remove ScrollShim mixin

c75c4a1

Convert Canvas to class component

87521bb

Fix ESLint errors

320b843

Add HOC and Keyboard utils

4fbe67c

Remove keyboardHandlerMixin from EditorConfig

d5ac522

Convert EditorContainer to a class component

e8ce7da

Remove displayName as it is inferred from the name of the class

1858fd0

Add scrollUtils

4c5d43d

Remove KeyboardHandlerMixin from ReactDataGrid component

d81b953

Extract ViewportScrollMixin logic into viewportUtils

87cef16

Move ViewportScrollMix code to Viewport component

d8bd2aa

Remove unused flow anotation

2c947e9

Remove GridScrollMixin

94c1c34

Add mising ReactDOM import

60f9b8c

Remove ColumnUtilsMixin from HeaderRow component

07f03b6

Convert HeaderRow to a class component

bfd939d

Remove displayName static property

e46a22d

Remove ColumnUtilsMixin from Row component

b523f20

Revert "Remove displayName static property"

b02df2e

This reverts commit e46a22d.

Revert "Convert HeaderRow to a class component"

23a4d77

This reverts commit bfd939d.

Revert "Remove displayName as it is inferred from the name of the class"

d562ec6

This reverts commit 1858fd0.

Revert "Convert EditorContainer to a class component"

9ca65f7

This reverts commit e8ce7da.

Revert "Fix ESLint errors"

010d7ed

This reverts commit 320b843.

Revert "Add scrollUtils"

0c08efa

This reverts commit 4c5d43d.

Revert "Convert Canvas to class component"

24e0dd3

This reverts commit 87521bb.

Add scrollUtils

4fbed5a

Remove MetricsComputatorMixin from the Grid component

d5980f1

(ReactDataGrid component adds the MetricsComputatorMixin so mixin methods are available to any child component via the context api, it does not need to be added to the Grid component)

amanmahajan7 changed the title ~~Remove Mixins~~ [WIP] Remove Mixins Jan 13, 2018

amanmahajan7 added the WIP label Jan 13, 2018

amanmahajan7 changed the title ~~[WIP] Remove Mixins~~ Remove Mixins Jan 13, 2018

amanmahajan7 added 2 commits January 16, 2018 13:16

Set _mounted to false on componentWillUnmount

141cfbc

Delete unused mixins

f6c2f45

amanmahajan7 commented Jan 16, 2018

View reviewed changes

amanmahajan7 commented Jan 17, 2018

View reviewed changes

Delete unused getRenderedColumnCount method

491a801

amanmahajan7 requested review from bakesteve and diogofcunha January 17, 2018 22:32

Add tests for KeyboardHandlerMixins methods

561ac33

amanmahajan7 removed the WIP label Jan 18, 2018

Use ref instead of React.findDOMNode

3097b81

chrisirhc mentioned this pull request Jan 20, 2018

[WIP] Remove mixins #967

Closed

3 tasks

malonecj reviewed Jan 22, 2018

View reviewed changes

amanmahajan7 added 5 commits January 22, 2018 10:35

Add unit tests for scrollUtils

438210b

Add unit tests for viewportUtils

4aee53f

Add unit tests for getRenderedColumnCount

22cfcf6

Add unit tests for getNextScrollState

9816dee

Add unit tests for keyboardUtils

365e6d1

amanmahajan7 removed the Needs Tests label Jan 22, 2018

Fix unit tests

1944481

malonecj approved these changes Jan 23, 2018

View reviewed changes

Fix cel navigation for locked cells: Check if the node containes the …

6d64e89

…class instead of strict equality

malonecj approved these changes Jan 25, 2018

View reviewed changes

malonecj merged commit 94bbd49 into master Jan 25, 2018

amanmahajan7 deleted the am-remove-mixins branch January 25, 2018 19:43

amanmahajan7 mentioned this pull request Jan 25, 2018

Convert createReactClass components to ES6 classes #1094

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove Mixins #1078

Remove Mixins #1078

Uh oh!

amanmahajan7 commented Jan 11, 2018 •

edited

Loading

Uh oh!

amanmahajan7 Jan 16, 2018

Uh oh!

amanmahajan7 Jan 17, 2018 •

edited

Loading

Uh oh!

amanmahajan7 Jan 17, 2018 •

edited

Loading

Uh oh!

amanmahajan7 Jan 17, 2018 •

edited

Loading

Uh oh!

chrisirhc commented Jan 20, 2018 •

edited

Loading

Uh oh!

malonecj left a comment

Uh oh!

amanmahajan7 commented Jan 22, 2018 •

edited

Loading

Uh oh!

malonecj Jan 23, 2018

Uh oh!

malonecj commented Jan 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Remove Mixins #1078

Remove Mixins #1078

Uh oh!

Conversation

amanmahajan7 commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

amanmahajan7 Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

amanmahajan7 Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanmahajan7 Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanmahajan7 Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisirhc commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malonecj left a comment

Choose a reason for hiding this comment

Uh oh!

amanmahajan7 commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

malonecj Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

malonecj commented Jan 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amanmahajan7 commented Jan 11, 2018 •

edited

Loading

amanmahajan7 Jan 17, 2018 •

edited

Loading

amanmahajan7 Jan 17, 2018 •

edited

Loading

amanmahajan7 Jan 17, 2018 •

edited

Loading

chrisirhc commented Jan 20, 2018 •

edited

Loading

amanmahajan7 commented Jan 22, 2018 •

edited

Loading