[Tabs] Migrate to hooks by oliviertassinari · Pull Request #16427 · mui/material-ui

oliviertassinari · 2019-06-29T20:35:21Z

Related to #15231.

mui-pr-bot · 2019-06-30T14:29:47Z

@material-ui/core: parsed: -1.06% 😍, gzip: -1.18% 😍

Details of bundle changes.

Comparing: 7050cf5...bf006b8

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-1.06%	-1.18%	331,257	327,757	91,531	90,452
@material-ui/core/Paper	-0.00%	0.00%	68,293	68,291	20,367	20,367
@material-ui/core/Paper.esm	0.00%	-0.04%	61,574	61,574	19,164	19,157
@material-ui/core/Popper	0.00%	0.00%	28,942	28,942	10,407	10,407
@material-ui/core/Textarea	0.00%	+0.04% 🔺	5,505	5,505	2,360	2,361
@material-ui/core/TrapFocus	0.00%	-0.13%	3,753	3,753	1,579	1,577
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	16,011	16,011	5,792	5,792
@material-ui/core/useMediaQuery	0.00%	0.00%	2,595	2,595	1,103	1,103
@material-ui/lab	-0.09%	-0.14%	140,155	140,022	43,484	43,423
@material-ui/styles	0.00%	+0.01% 🔺	51,699	51,699	15,339	15,341
@material-ui/system	0.00%	-0.02%	15,428	15,428	4,397	4,396
Button	0.00%	+0.05% 🔺	84,442	84,442	25,732	25,744
Modal	0.00%	+0.14% 🔺	14,533	14,533	5,094	5,101
Portal	0.00%	0.00%	3,471	3,471	1,574	1,574
Slider	0.00%	+0.02% 🔺	74,994	74,994	23,296	23,300
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	54,338	54,338	13,762	13,762
docs.main	0.00%	0.00%	647,028	647,028	203,913	203,914
packages/material-ui/build/umd/material-ui.production.min.js	-1.12%	-1.24%	303,536	300,130	87,156	86,077

Generated by 🚫 dangerJS against bf006b8

oliviertassinari · 2019-06-30T16:35:14Z

Migrate the tests to testing-library.

After migrating the Slider and the Tabs tests to testing-library, I have a bit more hindsight on the value this testing library bring. I definitely agree that the tests are in better shape. However, the migration task was time-consuming. I'm not sure that the gain worth it. We might have been better off spending this time on a different problem. I would propose that we prioritize removing the shallow tests over migrating the existing tests to testing-library first.

On a different note, I personally don't like the expect API. They are too many moving parts, take ).to.not.be.ok… . However, as Jest and testing-library are promoting it, we should migrate to it. I would encourage the usage of expect(B).to.equal(A) when possible. Maybe we could use a tool like https://github.com/devenbansod/jest-expect-codemod to automate the migration?

oliviertassinari · 2019-06-30T16:38:43Z

    "normalize-scroll-left": "^0.2.0",
    "popper.js": "^1.14.1",
    "prop-types": "^15.7.2",
-    "react-event-listener": "^0.6.6",


One less dependency :).

eps1lon · 2019-07-01T07:57:12Z

I would propose that we prioritize removing the shallow tests over migrating the existing tests to testing-library first.

Nobody proposed migrating the existing ones. I merely introduced another API in case tests need to be refactored for other reasons (strict mode, shallow removal etc).

take ).to.not.be.ok

Agree 💯 jest already identified this problem and reduced chainability to not only. Everything else is a single word improving grepability.

I wouldn't mind using jest-expect over chai's expect. I didn't even think about using it since I thought it was coupled with the test runner.

oliviertassinari · 2019-07-01T12:02:27Z

Nobody proposed migrating the existing ones. I merely introduced another API in case tests need to be refactored for other reasons (strict mode, shallow removal etc).

I believe that we (as a team) have introduced two ways of doing the same things (expect vs assert) and (enzyme vs testing-library). We should aim for simplicity, it's not OK to have two identical tools for the same job in the long run. However, we should also recognize that it's a necessary intermediate unideal state for improving the problems Material-UI tries to solve in the long term (providing the best possible React UI components).

So I think that we should progressively remove the assert and enzyme usage from the codebase. Does it make sense?

I wouldn't mind using jest-expect over chai's expect. I didn't even think about using it since I thought it was coupled with the test runner.

I would have a preference for jest expect as it might be more useful for people using our tests as inspiration. But, it's not very important from my perspective.

oliviertassinari · 2019-07-03T12:29:00Z

I will rebase the pull request to use the latest useEventCallback util and improve how the tests are written.

joshwooding · 2019-07-04T14:39:04Z

+    marginBottom: null,
+  });
+  const valueToIndex = React.useRef();
+  valueToIndex.current = new Map();


Is there a different with this vs const valueToIndex = React.useRef(new Map()) ?

I believe that the useRef first argument is used for the initial value (https://codesandbox.io/s/laughing-chandrasekhar-grk1r).
How will we reset the Map instance between two renders?

If it needs to reset between renders it makes sense to do it this way. Should we not calculate it when the children prop changes instead of every render?

I guess it might break ssr though I haven’t had a close look

children prop (unless explicitly memoised) is typically written inline, so it's likely to be a new reference each time anyway 🙂

So far, we are using the valueToIndex.current for:

Warning when providing a wrong value

Finding the selected tab item DOM node

Finding the selected tab item DOM node

Should just attach a ref to it in v5. I think we allow custom tab components which means it would be breaking. If not then we can do it now.

Actually thinking about it, for posterity & clarity maybe it's worth adding a comment to say why you're using a ref and not just a const valueToIndex = new Map() if you want it to reset every render?

Should just attach a ref to it in v5.

@eps1lon +1 for looking into what we can improve on the ref side in v5. It's something we could have implemented this way in the first place. For some reason, we didn't. To dive into why it wasn't done this way.

maybe it's worth adding a comment to say why you're using a ref and not just a const valueToIndex = new Map() if you want it to reset every render?

@NMinhNguyen Seems that we don't need the ref in the first place 🙃. I have just tried without, it's all right. Doing some more tests and committing.

NMinhNguyen · 2019-07-04T23:49:35Z


-    const { tabsMeta, tabMeta } = this.getTabsMeta(value, theme.direction);
-    let left = 0;
+    window.addEventListener('resize', handleResize);


Probably doesn't matter because this is exactly equivalent to the old implementation but in the past we had issues in multi-window (window.open()) environments where window was referring to the parent window, and so the most bulletproof way was to do element.ownerDocument.defaultView to get the right window object (i.e. child not parent). I'm aware of the ownerWindow helper https://github.com/mui-org/material-ui/blob/89687f38cae750650555772ba4d821c9084d8dfc/packages/material-ui/src/utils/ownerWindow.js#L5 btw. It's just when I last looked at the implementation, I wasn't too sure why it falls back the way it does - parentView isn't a DOM property from what I can tell, nor is parentWindow from the original dom-helpers.

@NMinhNguyen Interesting, we should probably use the ownerWindow and ownerDocument helpers for all our global event listeners to better support multi-window like use cases. We don't have a very strong support yet.

The parentView logic originally comes from react-bootstrap/dom-helpers@43fcc24#diff-991ea3fb4ae30d4ee9eda4d4f6ea9595. I would propose we remove it.

The fallback argument is also never used, we can use the same approach as in the ownerDocument helper.

Yeah, except it says parentWindow in dom-helpers 😅. Regardless, I don't think either of those properties exist: parentView, parentWindow. At least I couldn't find them when googling.

parentWindow seems to only be suported by IE and opera. Probably super old and just c&p (see over-polyfilling)

Ah you’re right. We could change parentView -> parentWindow or remove it altogether I guess.

dom-helpers supports older browsers (IE 8) than we do (IE 11). If we can remove parentView, it's even better.

@NMinhNguyen Do you want to hit this with a pull request? 💥

Yes, will do. I can wait until you merge this PR and then change window -> ownerWindow as well.

oliviertassinari assigned oliviertassinari and unassigned oliviertassinari Jun 29, 2019

oliviertassinari added the scope: tabs Changes related to the tabs. label Jun 29, 2019

joshwooding mentioned this pull request Jun 29, 2019

Migration to hooks #15231

Closed

29 tasks

oliviertassinari force-pushed the tabs-hook branch 4 times, most recently from f321919 to c1ced75 Compare June 30, 2019 14:22

oliviertassinari force-pushed the tabs-hook branch 2 times, most recently from f7efd43 to 78da4e8 Compare June 30, 2019 16:18

oliviertassinari added the core label Jun 30, 2019

oliviertassinari commented Jun 30, 2019

View reviewed changes

oliviertassinari force-pushed the tabs-hook branch 7 times, most recently from 714e1b6 to c867ede Compare June 30, 2019 22:16

oliviertassinari marked this pull request as ready for review June 30, 2019 22:16

oliviertassinari force-pushed the tabs-hook branch from c867ede to b26a224 Compare June 30, 2019 22:28

oliviertassinari requested review from eps1lon and joshwooding and removed request for joshwooding July 2, 2019 17:23

oliviertassinari force-pushed the tabs-hook branch from b26a224 to 9e622ee Compare July 3, 2019 14:29

oliviertassinari added the PR: good for merge label Jul 3, 2019

eps1lon reviewed Jul 4, 2019

View reviewed changes

Comment thread packages/material-ui/src/Tabs/Tabs.test.js Outdated

Comment thread packages/material-ui/src/Tabs/Tabs.test.js

oliviertassinari added PR: good for merge and removed PR: good for merge labels Jul 4, 2019

joshwooding approved these changes Jul 4, 2019

View reviewed changes

NMinhNguyen reviewed Jul 4, 2019

View reviewed changes

oliviertassinari added 4 commits July 5, 2019 17:01

[Tabs] Migrate to hooks

4dc28fa

rebase

46adbfe

sebastian review

9dee4ee

remove ref

bf006b8

oliviertassinari force-pushed the tabs-hook branch from 205c4b0 to bf006b8 Compare July 5, 2019 14:06

oliviertassinari merged commit 6a7daaa into mui:master Jul 5, 2019

oliviertassinari deleted the tabs-hook branch July 5, 2019 14:19

NMinhNguyen mentioned this pull request Jul 5, 2019

[Tabs] Use the correct window reference #16497

Merged

1 task

Uh oh!

Uh oh!

Conversation

oliviertassinari commented Jun 29, 2019

Uh oh!

mui-pr-bot commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eps1lon commented Jul 1, 2019

Uh oh!

oliviertassinari commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Jul 3, 2019

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Jul 4, 2019 • 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!

joshwooding Jul 4, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Jul 5, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

NMinhNguyen Jul 5, 2019 • 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!

oliviertassinari Jul 5, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Jul 5, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Jul 5, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mui-pr-bot commented Jun 30, 2019 •

edited

Loading

oliviertassinari commented Jun 30, 2019 •

edited

Loading

oliviertassinari commented Jul 1, 2019 •

edited

Loading

oliviertassinari Jul 4, 2019 •

edited

Loading

joshwooding Jul 4, 2019 •

edited

Loading

oliviertassinari Jul 5, 2019 •

edited

Loading

NMinhNguyen Jul 5, 2019 •

edited

Loading

oliviertassinari Jul 5, 2019 •

edited

Loading

oliviertassinari Jul 5, 2019 •

edited

Loading

oliviertassinari Jul 5, 2019 •

edited

Loading