[Tooltip] Convert to function component #15291

joshwooding · 2019-04-09T21:55:04Z

I have followed (at least) the PR section of the contributing guide.

Breaking Change

The child of the Tooltip needs to be able to hold a ref:

class Component extends React.Component {
  render() {
    return <div />
  }
}
-const MyComponent = props => <div {...props} />
+const MyComponent = React.forwardRef((props, ref) => <div ref={ref} {...props} />);
<Tooltip><Component /></Tooltip>
<Tooltip><MyComponent /></Tooltip>
<Tooltip><div /></Tooltip>

mui-pr-bot · 2019-04-09T22:00:01Z

@material-ui/core: parsed: -0.27% 😍, gzip: -0.09% 😍
@material-ui/lab: parsed: -1.15% 😍, gzip: -0.45% 😍

Details of bundle changes.

Comparing: 701a61d...f62e92a

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.27%	-0.09%	318,721	317,854	85,484	85,410
@material-ui/core/Paper	0.00%	-0.02%	67,276	67,276	19,958	19,954
@material-ui/core/Paper.esm	-0.00%	-0.02%	60,641	60,639	18,890	18,886
@material-ui/core/Popper	0.00%	-0.02%	34,906	34,906	11,868	11,866
@material-ui/core/styles/createMuiTheme	0.00%	+0.07% 🔺	15,898	15,898	5,769	5,773
@material-ui/core/useMediaQuery	0.00%	0.00%	2,463	2,463	1,041	1,041
@material-ui/lab	-1.15%	-0.45%	146,194	144,519	43,730	43,532
@material-ui/styles	0.00%	0.00%	50,831	50,831	15,013	15,013
@material-ui/system	0.00%	+0.10% 🔺	11,765	11,765	3,922	3,926
Button	-0.00%	+0.02% 🔺	88,560	88,558	26,490	26,496
Modal	0.00%	0.00%	82,538	82,539	24,835	24,835
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	50,908	50,908	11,210	11,210
docs.main	-0.12%	-0.02%	650,169	649,371	202,381	202,331
packages/material-ui/build/umd/material-ui.production.min.js	-0.30%	-0.09%	295,409	294,535	82,680	82,607

Generated by 🚫 dangerJS against f62e92a

packages/material-ui/src/Tooltip/Tooltip.js

packages/material-ui/src/Tooltip/Tooltip.test.js

oliviertassinari

We need to rebase for #15304 :)

packages/material-ui/src/Tooltip/Tooltip.js

joshwooding · 2019-04-12T20:36:16Z

Whoops, SpeedDial is broken.

packages/material-ui/src/Tooltip/Tooltip.js

eps1lon

About the order: We shouldn't order by the type of hook. We should order by concern. The goal should be to be as close as possible to https://twitter.com/prchdk/status/1056960391543062528

oliviertassinari · 2019-04-14T09:21:55Z

packages/material-ui/src/Tooltip/Tooltip.js

+  const handleRef = useForkRef(children.ref, handleOwnRef);
+
+  React.useEffect(() => {
+    if (childNode) {


We are mixing the if (x) { warning(message) } and warning(y, message) pattern. We should only use one strategy. React is introducing new helpers: facebook/react#15170. We can start to migrate to if(x) { warn(message) } if you prefer or stick to the existing convention.

I did want to refactor this but it's my kryptonite ... Long boolean expressions 😅

packages/material-ui/src/Tooltip/Tooltip.js

oliviertassinari · 2019-04-14T09:49:56Z

How do you guys want to handle the warning thing in the future, there is a wide range of different options. Maybe?

import React from 'react';

/* to package into it's own module */
const dedup = {};
function warning(message, type) {
  if (!dedup[type]) {
    React.warn(messasge);
    dedup[type] = 1;
  }
}

function MyComponent() {
  if (foo === bar) {
    warning('No, you should not bazzz', 'MyComponent');.
  }
}

plus https://github.com/oliviertassinari/babel-plugin-transform-dev-warning

eps1lon · 2019-04-14T09:52:44Z

Don't cache warnings unless it's actually an issue. Or rather don't cache anything unless you need to because caching is hard. Not sure what the issue is all of the sudden?

oliviertassinari · 2019-04-14T09:55:50Z

Warning spamming is a true issue. I'm proposing to use the same strategy that React uses for it's warning: raise them once. "Did you fix the warning? Great! Reload the page".

oliviertassinari · 2019-04-14T09:58:43Z

Another option:

import React from 'react';

/* to package into it's own module */
const dedup = new WeakMap();
function warning(condition, message) {
  const { current } = React.useRef({});
  if (!condition && !dedup.has(current)) {
    React.warn(messasge);
    dedup.set(current);
  }
}

function MyComponent() {
  warning(foo !== bar, 'No, you should not bazzz');.
}

eps1lon · 2019-04-14T10:01:49Z

Warning spamming is a true issue.

Sure. Could you showcase this? According to your own standards it shouldn't since nobody reported it.

oliviertassinari · 2019-04-14T10:23:51Z

According to your own standards, it shouldn't since nobody reported it.

Just to clarify, my standard is all about experimenting and gaining knowledge 🔬. The bench is skewed here. We are already deduplicating the Tooltip warning in a did mount. We can't conclude.

Could you showcase this?

We can look at our past experimentations and the solution others are using (React), if often reveal a lot of interesting things (why I benchmark as much as I can).

oliviertassinari · 2019-04-14T10:37:34Z

The stack trace is present: https://github.com/facebook/react/pull/15170/files#diff-f49694827a75d846a7c34f03316ecaf1R177. So it's only a dead code elimination & deduplication problem.

eps1lon · 2019-04-14T10:41:53Z

Let's continue this in the actual issue. This isn't tied to "[Tooltip] Convert to function component".

oliviertassinari · 2019-04-14T13:40:37Z

packages/material-ui/src/Tooltip/Tooltip.js

-      );
-    }
+    warning(
+      !(


💯 That's is, maybe we can write our own warning method that uses warn internally but reverse the condition logic. It's more intuitive. Maybe we don't need a different if branch. What we need is a direct condition logic, not a reversed one.

oliviertassinari · 2019-04-15T09:46:46Z

Well done! Let's mature the warning reflection before committing to any change.

joshwooding added new feature New feature or request component: tooltip This is the name of the generic UI component, not the React module! labels Apr 9, 2019

joshwooding force-pushed the tooltip-function-component branch 2 times, most recently from e554936 to c4f745a Compare April 9, 2019 22:14

joshwooding mentioned this pull request Apr 9, 2019

Migration to hooks #15231

Closed

29 tasks

joshwooding force-pushed the tooltip-function-component branch from 87862a6 to 17762f2 Compare April 9, 2019 22:57

joshwooding requested a review from eps1lon April 9, 2019 22:58

joshwooding commented Apr 9, 2019

View reviewed changes

packages/material-ui/src/Tooltip/Tooltip.js Outdated Show resolved Hide resolved

joshwooding marked this pull request as ready for review April 10, 2019 00:09

eps1lon reviewed Apr 10, 2019

View reviewed changes

eps1lon added the breaking change label Apr 10, 2019

oliviertassinari mentioned this pull request Apr 10, 2019

[Tooltip] Suppress disabled button warning when controlled #15304

Merged

1 task

joshwooding commented Apr 10, 2019

View reviewed changes

packages/material-ui/src/Tooltip/Tooltip.test.js Outdated Show resolved Hide resolved

oliviertassinari reviewed Apr 11, 2019

View reviewed changes

packages/material-ui/src/Tooltip/Tooltip.js Outdated Show resolved Hide resolved

eps1lon reviewed Apr 11, 2019

View reviewed changes

packages/material-ui/src/Tooltip/Tooltip.js Outdated Show resolved Hide resolved

packages/material-ui/src/Tooltip/Tooltip.js Outdated Show resolved Hide resolved

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Apr 12, 2019

joshwooding added 7 commits April 12, 2019 18:55

[Tooltip] Convert to function component

2de6ce0

Remove unwrap usage

3df2d60

Remove ref function

ba36c8b

I think I've done this right

514b322

Add forceUpdate on children mount

52f8ff6

Code review

4fcfd13

Code review

bd66482

joshwooding force-pushed the tooltip-function-component branch from 8459d60 to bd66482 Compare April 12, 2019 18:02

Remove mountRef

345458b

joshwooding removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Apr 12, 2019

eps1lon self-assigned this Apr 13, 2019

Fix mistake

6626831

joshwooding requested review from eps1lon and oliviertassinari April 14, 2019 00:40

joshwooding unassigned eps1lon Apr 14, 2019

Re-order

f74dcbf

eps1lon reviewed Apr 14, 2019

View reviewed changes

packages/material-ui/src/Tooltip/Tooltip.js Outdated Show resolved Hide resolved

eps1lon reviewed Apr 14, 2019

View reviewed changes

oliviertassinari reviewed Apr 14, 2019

View reviewed changes

eps1lon mentioned this pull request Apr 14, 2019

Improve the warning strategy #15343

Closed

Boolean fun

541fc2f

oliviertassinari reviewed Apr 14, 2019

View reviewed changes

oliviertassinari approved these changes Apr 14, 2019

View reviewed changes

oliviertassinari added the PR: good for merge label Apr 14, 2019

joshwooding mentioned this pull request Apr 14, 2019

[core] Remove RootRef usage #15347

Merged

1 task

Merge branch 'next' into tooltip-function-component

f62e92a

oliviertassinari merged commit 8e5d48b into mui:next Apr 15, 2019

joshwooding deleted the tooltip-function-component branch April 15, 2019 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tooltip] Convert to function component #15291

[Tooltip] Convert to function component #15291

joshwooding commented Apr 9, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Apr 9, 2019 •

edited

Loading

oliviertassinari left a comment

joshwooding commented Apr 12, 2019

eps1lon left a comment

oliviertassinari Apr 14, 2019

joshwooding Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 14, 2019 •

edited

Loading

eps1lon commented Apr 14, 2019

oliviertassinari commented Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 14, 2019 •

edited

Loading

eps1lon commented Apr 14, 2019

oliviertassinari commented Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 14, 2019

eps1lon commented Apr 14, 2019

oliviertassinari Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 15, 2019

[Tooltip] Convert to function component #15291

[Tooltip] Convert to function component #15291

Conversation

joshwooding commented Apr 9, 2019 • edited by oliviertassinari Loading

Breaking Change

mui-pr-bot commented Apr 9, 2019 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

joshwooding commented Apr 12, 2019

eps1lon left a comment

Choose a reason for hiding this comment

oliviertassinari Apr 14, 2019

Choose a reason for hiding this comment

joshwooding Apr 14, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Apr 14, 2019 • edited Loading

eps1lon commented Apr 14, 2019

oliviertassinari commented Apr 14, 2019 • edited Loading

oliviertassinari commented Apr 14, 2019 • edited Loading

eps1lon commented Apr 14, 2019

oliviertassinari commented Apr 14, 2019 • edited Loading

oliviertassinari commented Apr 14, 2019

eps1lon commented Apr 14, 2019

oliviertassinari Apr 14, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Apr 15, 2019

joshwooding commented Apr 9, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Apr 9, 2019 •

edited

Loading

joshwooding Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 14, 2019 •

edited

Loading

oliviertassinari commented Apr 14, 2019 •

edited

Loading

oliviertassinari Apr 14, 2019 •

edited

Loading