[ButtonBase] Convert to function component by eps1lon · Pull Request #15716 · mui/material-ui

eps1lon · 2019-05-15T20:57:24Z

Buttons playground

TODO:

Fix tests
Switching from testing implementation details via instance access to css classes. It's a bit better since these are coupled with the appearance but it's still questionable if unit tests are appropriate here. Fixtures and manual testing are probably more appropriate but lets see how this develops.
move defaultProps resolution to render

Future Work:

re-evaluate useFocusVisible API (move state into hook; prepare in ref phase etc)
Enter key doesn't cause pulsate

mui-pr-bot · 2019-05-15T21:07:06Z

@material-ui/core: parsed: -0.47% 😍, gzip: -0.15% 😍
@material-ui/lab: parsed: -1.06% 😍, gzip: -0.35% 😍

Details of bundle changes.

Comparing: a15f6ad...811e413

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.47%	-0.15%	316,713	315,226	86,546	86,417
@material-ui/core/Paper	0.00%	-0.01%	67,869	67,869	20,165	20,163
@material-ui/core/Paper.esm	0.00%	-0.01%	61,152	61,152	18,955	18,954
@material-ui/core/Popper	0.00%	-0.03%	28,740	28,740	10,354	10,351
@material-ui/core/Textarea	0.00%	-0.04%	5,513	5,513	2,383	2,382
@material-ui/core/TrapFocus	0.00%	+0.06% 🔺	3,744	3,744	1,580	1,581
@material-ui/core/styles/createMuiTheme	0.00%	+0.05% 🔺	15,960	15,960	5,779	5,782
@material-ui/core/useMediaQuery	0.00%	+0.10% 🔺	2,106	2,106	974	975
@material-ui/lab	-1.06%	-0.35%	140,518	139,031	42,850	42,699
@material-ui/styles	0.00%	-0.03%	51,353	51,353	15,184	15,180
@material-ui/system	0.00%	+0.07% 🔺	14,458	14,458	4,175	4,178
Button	-2.01%	-0.71%	85,781	84,061	25,765	25,583
Modal	0.00%	-0.18%	20,344	20,344	6,698	6,686
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	47,836	47,836	10,920	10,920
docs.main	-0.27%	-0.16%	655,513	653,763	205,874	205,546
packages/material-ui/build/umd/material-ui.production.min.js	-0.47%	-0.23%	295,580	294,201	84,056	83,865

Generated by 🚫 dangerJS against 811e413

eps1lon · 2019-05-16T14:06:12Z

+ * no jsdom
+ */
+// eslint-disable-next-line camelcase
+React.useLayoutEffect = function unstable_useIsomorphicLayoutEffect(fn, deps) {


Tried creating a separate ssr test suite but this was too much work for a couple of tests. While this isn't pretty it works.

I have used process.env.NODE_ENV !== 'test' in NoSsr.js. I believe we should consolidate around one solution.

But this will use useLayoutEffect in properly setup ssr test setups i.e. for test setups without JSDOm but with NODE_ENV=test this will trigger useLayoutEffect warnings

eps1lon · 2019-05-16T14:14:03Z

-    describe('avoids multiple keydown presses', () => {
+  describe('event: keydown', () => {
+    // eslint-disable-next-line mocha/no-skipped-tests
+    describe.skip('avoids multiple keydown presses', () => {


The behavior is not apparent to me (due to no information in the test description).

It probably wants to avoid repeated keydowns causing a ripple but I'm not sure what the benefit is. If somebody presses and holds Space and then Enter another click will be dispatched at which point we probably should create a new Ripple. Not even including that only space triggers a ripple currently.

I would skip this for now and explore in a follow-up if we can get rid of the whole "avoid repeated keydown" logic.

due to no information in the test description

I think that the test assertions and test actions should be what yield most of the information. The description is not really important.
Same as you, I don't understand what this test is about. Let's just kill it. It seems it was seeded in a 100% test coverage effort.

I think that the test assertions and test actions should be what yield most of the information

So in face of a cryptic test with a bad description you think tests have more information than descriptions? Why do you think these descriptions exist? Tests are an implementation detail. The description describes the API.

I'll keep it for now because I want to specifically fail it in a later PR: repeated keydown should ripple because you could press spacebar and then enter. Currently enter doesn't create a ripple but does fire a click.

Yes, the test description is the last thing I look at when working with the tests. The shorter & generic the descriptions can be, the better. It's less surface area for an outdated description. I'm not saying the description is useless, I'm saying that the test detail is more important. It's what we should most care about. it's our insurance, the more descriptive they are, the better.

Oh, you are right, Space triggers the ripple, but Enter doesn't. I agree that something seems off.

outdated description

Descriptions rarely become outdated because that would mean you introduced a breaking change. This happens far less than a human reading a test.

oliviertassinari · 2019-05-16T19:15:45Z

+ * no jsdom
+ */
+// eslint-disable-next-line camelcase
+React.useLayoutEffect = function unstable_useIsomorphicLayoutEffect(fn, deps) {


I have used process.env.NODE_ENV !== 'test' in NoSsr.js. I believe we should consolidate around one solution.

oliviertassinari · 2019-05-16T19:26:28Z

-    describe('avoids multiple keydown presses', () => {
+  describe('event: keydown', () => {
+    // eslint-disable-next-line mocha/no-skipped-tests
+    describe.skip('avoids multiple keydown presses', () => {


due to no information in the test description

I think that the test assertions and test actions should be what yield most of the information. The description is not really important.
Same as you, I don't understand what this test is about. Let's just kill it. It seems it was seeded in a 100% test coverage effort.

oliviertassinari · 2019-05-16T19:39:57Z

+  useEnhancedEffect(() => {
+    ref.current = fn;
+  });
+  return React.useCallback(event => (0, ref.current)(event), []);


Suggested change

return React.useCallback(event => (0, ref.current)(event), []);

return React.useCallback(event => ref.current(event), []);

https://stackoverflow.com/a/32275207/3406963
Also: The source uses comma operator.

I didn't know that. It seems that in strict mode, the context is undefined. Does it have any practical implication?

I don't have the full picture but I would trust Sophie to not post wrong code. Better stick with the source code instead of changing things we do not fully understand.

Co-Authored-By: Olivier Tassinari <[email protected]>

Makes it actually invariant once loaded

oliviertassinari · 2019-05-17T19:12:04Z

+  useEnhancedEffect(() => {
+    ref.current = fn;
+  });
+  return React.useCallback(event => (0, ref.current)(event), []);


I didn't know that. It seems that in strict mode, the context is undefined. Does it have any practical implication?

oliviertassinari · 2019-05-18T03:13:53Z

  focusVisible: {},
 };

+const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;


You might have already answered this point, it's hard for me to follow the past discussions in the GitHub UI. I'm wondering if we can end up in a weird case where the event is triggered but the handled hasn't been updated yet.
Why not follow the pattern of the linked comment?

Suggested change

const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;

const useEnhancedEffect = React.useEffect;

I we can do that, we can probably remove unstable_useIsomorphicLayoutEffect too.

GitHub seemed to have lost part of my answer when it duplicated the conversation.

We can't use useEffect because their priority might be lower than user input events. Which means you could call a stale value instead. This has to be done synchronous i.e. with useLayoutEffect

Why not follow the pattern of the linked comment?

I don't understand. The linked comment is not using useEffect but useLayoutEffect. The rest is just the usual "trick react into thinking this is OK for ssr".

oliviertassinari · 2019-05-18T03:19:19Z

-    describe('avoids multiple keydown presses', () => {
+  describe('event: keydown', () => {
+    // eslint-disable-next-line mocha/no-skipped-tests
+    describe.skip('avoids multiple keydown presses', () => {


Yes, the test description is the last thing I look at when working with the tests. The shorter & generic the descriptions can be, the better. It's less surface area for an outdated description. I'm not saying the description is useless, I'm saying that the test detail is more important. It's what we should most care about. it's our insurance, the more descriptive they are, the better.

Oh, you are right, Space triggers the ripple, but Enter doesn't. I agree that something seems off.

oliviertassinari · 2019-05-19T10:56:45Z

Well done!

eps1lon added performance component: ButtonBase The React component. labels May 15, 2019

eps1lon commented May 15, 2019

View reviewed changes

Comment thread packages/material-ui/src/ButtonBase/ButtonBase.js

eps1lon commented May 15, 2019

View reviewed changes

Comment thread packages/material-ui/src/ButtonBase/ButtonBase.js

eps1lon commented May 15, 2019

View reviewed changes

Comment thread packages/material-ui/src/ButtonBase/ButtonBase.js

eps1lon commented May 15, 2019

View reviewed changes

Comment thread packages/material-ui/src/ButtonBase/ButtonBase.js

joshwooding mentioned this pull request May 15, 2019

Migration to hooks #15231

Closed

29 tasks

eps1lon force-pushed the refactor/ButtonBase/function-component branch from b51613b to 43ad125 Compare May 15, 2019 22:45

eps1lon added 2 commits May 16, 2019 00:51

[ButtonBase] Convert to function component

c410842

Fix focusVisible tests

aae1e9e

eps1lon force-pushed the refactor/ButtonBase/function-component branch from 43ad125 to aae1e9e Compare May 15, 2019 22:53

Fix Tests

a32f805

eps1lon commented May 16, 2019

View reviewed changes

eps1lon added 2 commits May 16, 2019 16:07

Revert wrong node tests

9b683d2

Fix useLayoutEffect warnings

19c20f7

eps1lon force-pushed the refactor/ButtonBase/function-component branch from 409f44f to 19c20f7 Compare May 16, 2019 14:07

eps1lon commented May 16, 2019

View reviewed changes

Fix browser tests

a6b46b3

eps1lon requested review from joshwooding and oliviertassinari May 16, 2019 14:55

oliviertassinari reviewed May 16, 2019

View reviewed changes

eps1lon and others added 3 commits May 16, 2019 22:32

Apply suggestions from code review

710ca03

Co-Authored-By: Olivier Tassinari <[email protected]>

consistent rippleRef.current access

28700b8

Move default props resolution into render

5606510

joshwooding reviewed May 16, 2019

View reviewed changes

Comment thread packages/material-ui/src/ButtonBase/ButtonBase.js Outdated

eps1lon added 4 commits May 17, 2019 10:26

Fix handleServer return type

24233f1

useEffect -> useImperativeHandle

6f35171

Remove server branch for useRippleHandler

b5b41de

Move useEnhancedEffect outside of scope

12f50c0

Makes it actually invariant once loaded

<> -> <React.Fragment>

811e413

eps1lon force-pushed the refactor/ButtonBase/function-component branch from b22d7b8 to 811e413 Compare May 17, 2019 09:12

oliviertassinari reviewed May 18, 2019

View reviewed changes

oliviertassinari approved these changes May 18, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label May 18, 2019

oliviertassinari merged commit 5531c38 into mui:next May 19, 2019

eps1lon deleted the refactor/ButtonBase/function-component branch May 20, 2019 09:43

	return React.useCallback(event => (0, ref.current)(event), []);
	return React.useCallback(event => ref.current(event), []);

	const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;
	const useEnhancedEffect = React.useEffect;

Uh oh!

Uh oh!

Conversation

eps1lon commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mui-pr-bot commented May 15, 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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eps1lon May 16, 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!

Uh oh!

Uh oh!

Uh oh!

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!

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 commented May 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eps1lon commented May 15, 2019 •

edited

Loading

mui-pr-bot commented May 15, 2019 •

edited

Loading

eps1lon May 16, 2019 •

edited

Loading