From 079783c68c83c4d8194c5c671936b86fcc98d2a9 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Wed, 10 Apr 2019 14:27:36 +0200 Subject: [PATCH 01/10] [docs] Include warning sign for elementTypeAcceptingRef --- docs/src/modules/utils/generateMarkdown.js | 2 +- pages/api/button-base.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/src/modules/utils/generateMarkdown.js b/docs/src/modules/utils/generateMarkdown.js index b9dfa6cee8f3b5..8b62eeea11d4dd 100644 --- a/docs/src/modules/utils/generateMarkdown.js +++ b/docs/src/modules/utils/generateMarkdown.js @@ -152,7 +152,7 @@ function generatePropDescription(prop) { let notes = ''; if (isElementTypeAcceptingRefProp(type)) { - notes += '
[Needs to be able to hold a ref](/guides/composition#caveat-with-refs).'; + notes += '
⚠️ [Needs to be able to hold a ref](/guides/composition#caveat-with-refs).'; } return `${deprecated}${jsDocText}${signature}${notes}`; diff --git a/pages/api/button-base.md b/pages/api/button-base.md index 68f3e265b6cef8..0f97e60d0f0cdd 100644 --- a/pages/api/button-base.md +++ b/pages/api/button-base.md @@ -25,7 +25,7 @@ It contains a load of style reset and some focus/ripple logic. | centerRipple | bool | false | If `true`, the ripples will be centered. They won't start at the cursor interaction position. | | children | node | | The content of the component. | | classes | object | | Override or extend the styles applied to the component. See [CSS API](#css) below for more details. | -| component | element type | 'button' | The component used for the root node. Either a string to use a DOM element or a component.
[Needs to be able to hold a ref](/guides/composition#caveat-with-refs). | +| component | element type | 'button' | The component used for the root node. Either a string to use a DOM element or a component.
⚠️ [Needs to be able to hold a ref](/guides/composition#caveat-with-refs). | | disabled | bool | | If `true`, the base button will be disabled. | | disableRipple | bool | false | If `true`, the ripple effect will be disabled. | | disableTouchRipple | bool | false | If `true`, the touch ripple effect will be disabled. | From ae075c9955a0c67aa36d1b8a27222ce3e780097b Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Wed, 10 Apr 2019 15:39:30 +0200 Subject: [PATCH 02/10] [docs] Enhance ref section - include children (used in Slide and future Tooltip) - be more specific in strict mode caveat --- .../pages/guides/composition/composition.md | 70 +++++++++++++------ packages/material-ui/src/Slide/Slide.js | 5 +- pages/api/slide.md | 2 +- 3 files changed, 54 insertions(+), 23 deletions(-) diff --git a/docs/src/pages/guides/composition/composition.md b/docs/src/pages/guides/composition/composition.md index 7b95cf6559df2c..05c109a55f972e 100644 --- a/docs/src/pages/guides/composition/composition.md +++ b/docs/src/pages/guides/composition/composition.md @@ -127,27 +127,46 @@ Here is a demo with [React Router DOM](https://github.com/ReactTraining/react-ro You can find the details in the [TypeScript guide](/guides/typescript#usage-of-component-property). -### Caveat with refs +## Caveat with refs -Some components such as `ButtonBase` (and therefore `Button`) require access to the -underlying DOM node. This was previously done with `ReactDOM.findDOMNode(this)`. - However `findDOMNode` was deprecated (which disqualifies its usage in React's concurrent mode) -in favour of component refs and ref forwarding. +This section covers caveats when using a custom component as `children` or for the +`component` prop. -It is therefore necessary that the component you pass to the `component` prop -can hold a ref. This includes: +Some of the components need access to the DOM node. This was previously possible +by using `ReactDOM.findDOMNode`. This function is deprecated in favor of `ref` and +ref forwarding. However, only the following component types can be given a `ref`: -- class components -- ref forwarding components (`React.forwardRef`) -- built-in components e.g. `div` or `a` +* Any Material-UI component +* class components i.e. `React.Component` or `React.PureComponent` +* built-in (or host) components e.g. `div` or `button` +* [React.forwardRef components](https://reactjs.org/docs/react-api.html#reactforwardref) +* [React.lazy components](https://reactjs.org/docs/react-api.html#reactlazy) +* [React.memo components](https://reactjs.org/docs/react-api.html#reactmemo) -If this is not the case we will issue a prop type warning similar to: +If you don't use one of the above types in Material-UI components you might see a warning from +React in your console similar to +> Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()? + +Be aware that you still get this warnings for lazy and memo components if their +wrapped component can't hold a ref. + +In some instances we issue an additional warning to help debugging similar to > Invalid prop `component` supplied to `ComponentName`. Expected an element type that can hold a ref. -In addition React will issue a warning. -You can fix this warning by using `React.forwardRef`. Learn more about it in -[this section in the official React docs](https://reactjs.org/docs/forwarding-refs.html). +We will only cover the two most common use cases. For more information see [this section in the official React docs](https://reactjs.org/docs/forwarding-refs.html). + +```diff +- const MyButton = props =>
++ const MyButton = React.forwardRef((props, ref) =>
) + - - Scroll around this container to experiment with flip and preventOverflow - modifiers. - - - {arrow ? : null} - - {"Use Google's location service?"} - - - Let Google help apps determine location. - - - - - - - - -
- - + +
+ + + Scroll around this container to experiment with flip and preventOverflow modifiers. + + + {arrow ? : null} + + {"Use Google's location service?"} + + Let Google help apps determine location. + + + + + + + +
+
diff --git a/packages/material-ui/src/RootRef/RootRef.js b/packages/material-ui/src/RootRef/RootRef.js index 281b27923a0604..1763bcb18f8fd3 100644 --- a/packages/material-ui/src/RootRef/RootRef.js +++ b/packages/material-ui/src/RootRef/RootRef.js @@ -5,6 +5,13 @@ import { exactProp } from '@material-ui/utils'; import { setRef } from '../utils/reactHelpers'; /** + * ⚠️⚠️⚠️ + * If you want the DOM element of a Material-UI component check out + * [/getting-started/faq/#how-can-i-access-the-dom-element](FAQ: How can I access the DOM element?) + * first. + * + * This component uses `findDOMNode` which is deprecated in React.StrictMode. + * * Helper component to allow attaching a ref to a * wrapped element to access the underlying DOM element. * diff --git a/packages/material-ui/test/integration/MenuList.test.js b/packages/material-ui/test/integration/MenuList.test.js index 4dbe80c6b608f9..5aeee978c5b0e4 100644 --- a/packages/material-ui/test/integration/MenuList.test.js +++ b/packages/material-ui/test/integration/MenuList.test.js @@ -3,7 +3,6 @@ import { assert } from 'chai'; import { spy } from 'sinon'; import MenuList from 'packages/material-ui/src/MenuList'; import MenuItem from 'packages/material-ui/src/MenuItem'; -import RootRef from 'packages/material-ui/src/RootRef'; import { createMount } from 'packages/material-ui/src/test-utils'; function FocusOnMountMenuItem(props) { @@ -11,11 +10,7 @@ function FocusOnMountMenuItem(props) { React.useLayoutEffect(() => { listItemRef.current.focus(); }, []); - return ( - - - - ); + return ; } function assertMenuItemTabIndexed(wrapper, tabIndexed) { diff --git a/pages/api/root-ref.md b/pages/api/root-ref.md index 8837e6905caf1a..b17acb83a8cfe9 100644 --- a/pages/api/root-ref.md +++ b/pages/api/root-ref.md @@ -12,6 +12,13 @@ filename: /packages/material-ui/src/RootRef/RootRef.js import RootRef from '@material-ui/core/RootRef'; ``` +⚠️⚠️⚠️ +If you want the DOM element of a Material-UI component check out +[/getting-started/faq/#how-can-i-access-the-dom-element](FAQ: How can I access the DOM element?) +first. + +This component uses `findDOMNode` which is deprecated in React.StrictMode. + Helper component to allow attaching a ref to a wrapped element to access the underlying DOM element. From e873cf58b26e13142bb7604f3905b6fe1dedb94f Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Thu, 11 Apr 2019 14:29:25 +0200 Subject: [PATCH 06/10] [docs] Include ref forwarding in API design --- docs/src/pages/guides/api/api.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/src/pages/guides/api/api.md b/docs/src/pages/guides/api/api.md index 2b7e424d3e7bc7..f898ed26ef3566 100644 --- a/docs/src/pages/guides/api/api.md +++ b/docs/src/pages/guides/api/api.md @@ -136,3 +136,10 @@ The Material-UI components use a combination of the two approaches according to - An *enum* is used when **> 2** degrees of freedom are required, or if there is the possibility that additional degrees of freedom may be required in the future. Going back to the previous button example; since it requires 3 degrees of freedom, we use an *enum*. + +### Ref + +The `ref` is forwarded to the root element. This means that without changing the rendered root element +via `component` prop it is forwarded to the outermost DOM element that component +renders. If you pass a different component via `component` prop the ref will be attached +to that component instead. \ No newline at end of file From abccfd7de03939343c9428007de75c3685102893 Mon Sep 17 00:00:00 2001 From: Matt Date: Sat, 13 Apr 2019 16:49:16 +0200 Subject: [PATCH 07/10] Apply suggestions from @mbrookes code review Co-Authored-By: eps1lon --- docs/src/pages/guides/api/api.md | 8 ++++---- docs/src/pages/guides/composition/composition.md | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/src/pages/guides/api/api.md b/docs/src/pages/guides/api/api.md index f898ed26ef3566..a45bc734d0df75 100644 --- a/docs/src/pages/guides/api/api.md +++ b/docs/src/pages/guides/api/api.md @@ -139,7 +139,7 @@ Going back to the previous button example; since it requires 3 degrees of freedo ### Ref -The `ref` is forwarded to the root element. This means that without changing the rendered root element -via `component` prop it is forwarded to the outermost DOM element that component -renders. If you pass a different component via `component` prop the ref will be attached -to that component instead. \ No newline at end of file +The `ref` is forwarded to the root element. This means that, without changing the rendered root element +via the `component` prop, it is forwarded to the outermost DOM element that which component +renders. If you pass a different component via the `component` prop the ref will be attached +to that component instead. diff --git a/docs/src/pages/guides/composition/composition.md b/docs/src/pages/guides/composition/composition.md index 05c109a55f972e..d9f9138a93b6f5 100644 --- a/docs/src/pages/guides/composition/composition.md +++ b/docs/src/pages/guides/composition/composition.md @@ -143,14 +143,14 @@ ref forwarding. However, only the following component types can be given a `ref` * [React.lazy components](https://reactjs.org/docs/react-api.html#reactlazy) * [React.memo components](https://reactjs.org/docs/react-api.html#reactmemo) -If you don't use one of the above types in Material-UI components you might see a warning from -React in your console similar to +If you don't use one of the above types when using your components in conjunction with Material-UI, you might see a warning from +React in your console similar to: > Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()? -Be aware that you still get this warnings for lazy and memo components if their +Be aware that you will still get this warning for `lazy` and `memo` components if their wrapped component can't hold a ref. -In some instances we issue an additional warning to help debugging similar to +In some instances we issue an additional warning to help debugging, similar to: > Invalid prop `component` supplied to `ComponentName`. Expected an element type that can hold a ref. From 6695bd33e89b36879665d8479e7bf0af400c5ad8 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Sat, 13 Apr 2019 17:18:01 +0200 Subject: [PATCH 08/10] Apply suggestions from code review buit-in, host -> DOM Since we only support react-dom renderer the host is equivalent to the DOM. --- docs/src/pages/getting-started/faq/faq.md | 2 +- docs/src/pages/guides/composition/composition.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/src/pages/getting-started/faq/faq.md b/docs/src/pages/getting-started/faq/faq.md index ee1b9681347fda..d250577dae6077 100644 --- a/docs/src/pages/getting-started/faq/faq.md +++ b/docs/src/pages/getting-started/faq/faq.md @@ -127,7 +127,7 @@ export default withTheme(withStyles(styles)(Modal)); ## How can I access the DOM element? All Material-UI components that should render something in the DOM forward their -ref to the underlying built-in component. This means that you can get DOM elements +ref to the underlying DOM component. This means that you can get DOM elements by reading the ref attached to Material-UI components: ```jsx diff --git a/docs/src/pages/guides/composition/composition.md b/docs/src/pages/guides/composition/composition.md index d9f9138a93b6f5..07eab03d8d6c6f 100644 --- a/docs/src/pages/guides/composition/composition.md +++ b/docs/src/pages/guides/composition/composition.md @@ -138,7 +138,7 @@ ref forwarding. However, only the following component types can be given a `ref` * Any Material-UI component * class components i.e. `React.Component` or `React.PureComponent` -* built-in (or host) components e.g. `div` or `button` +* DOM (or host) components e.g. `div` or `button` * [React.forwardRef components](https://reactjs.org/docs/react-api.html#reactforwardref) * [React.lazy components](https://reactjs.org/docs/react-api.html#reactlazy) * [React.memo components](https://reactjs.org/docs/react-api.html#reactmemo) @@ -177,7 +177,7 @@ the description will link to this section. If you use class components for the cases described above you will still see warnings in `React.StrictMode` and `React.unstable_ConcurrentMode`. We use `ReactDOM.findDOMNode` internally for backwards compatibility. You can use `React.forwardRef` -and a designated prop in your class component to forward the `ref` to a built-in +and a designated prop in your class component to forward the `ref` to a DOM component. Doing so should not trigger any more warnings related to the deprecation of `ReactDOM.findDOMNode`. From 57f1c3b4e2c89443e60dafc9022b2e7c06712a68 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Sat, 13 Apr 2019 21:27:08 +0200 Subject: [PATCH 09/10] Apply suggestions from code review Co-Authored-By: eps1lon --- docs/src/pages/getting-started/faq/faq.md | 2 +- docs/src/pages/guides/composition/composition.md | 2 +- packages/material-ui/src/ClickAwayListener/ClickAwayListener.js | 2 +- packages/material-ui/src/Slide/Slide.js | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/src/pages/getting-started/faq/faq.md b/docs/src/pages/getting-started/faq/faq.md index d250577dae6077..13997e67363537 100644 --- a/docs/src/pages/getting-started/faq/faq.md +++ b/docs/src/pages/getting-started/faq/faq.md @@ -140,7 +140,7 @@ const element = ref.current; ``` If you're not sure if the Material-UI component in question forwards its ref you -can check the API documentation under "Props" e.g. the [/api/Button#props](Button API) +can check the API documentation under "Props" e.g. the [/api/button/#props](Button API) includes > The ref is forwarded to the root element. diff --git a/docs/src/pages/guides/composition/composition.md b/docs/src/pages/guides/composition/composition.md index 07eab03d8d6c6f..5519d55f44cb1d 100644 --- a/docs/src/pages/guides/composition/composition.md +++ b/docs/src/pages/guides/composition/composition.md @@ -136,7 +136,7 @@ Some of the components need access to the DOM node. This was previously possible by using `ReactDOM.findDOMNode`. This function is deprecated in favor of `ref` and ref forwarding. However, only the following component types can be given a `ref`: -* Any Material-UI component +- Any Material-UI component * class components i.e. `React.Component` or `React.PureComponent` * DOM (or host) components e.g. `div` or `button` * [React.forwardRef components](https://reactjs.org/docs/react-api.html#reactforwardref) diff --git a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js index 76ce1c0f20b715..a610dd24a41cda 100644 --- a/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js +++ b/packages/material-ui/src/ClickAwayListener/ClickAwayListener.js @@ -97,7 +97,7 @@ ClickAwayListener.propTypes = { /** * The wrapped element. * - * ⚠️The component used as a child [must be able to hold a ref](/guides/composition#children). + * ⚠️The component used as a child [must be able to hold a ref](/guides/composition/#children). */ children: PropTypes.element.isRequired, /** diff --git a/packages/material-ui/src/Slide/Slide.js b/packages/material-ui/src/Slide/Slide.js index ae9674cd70fab6..14b1dc8007e064 100644 --- a/packages/material-ui/src/Slide/Slide.js +++ b/packages/material-ui/src/Slide/Slide.js @@ -239,7 +239,7 @@ Slide.propTypes = { /** * A single child content element. * - * ⚠️The component used as a child [must be able to hold a ref](/guides/composition#children). + * ⚠️The component used as a child [must be able to hold a ref](/guides/composition/#children). */ children: PropTypes.element, /** From 2060a078d3e63b6400ebd3f68bb72287ef469bca Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Sat, 13 Apr 2019 23:42:22 +0200 Subject: [PATCH 10/10] Apply suggestions from code review --- docs/src/modules/utils/generateMarkdown.js | 2 +- pages/api/button-base.md | 2 +- pages/api/click-away-listener.md | 2 +- pages/api/slide.md | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/src/modules/utils/generateMarkdown.js b/docs/src/modules/utils/generateMarkdown.js index 8b62eeea11d4dd..f81a189e9826b2 100644 --- a/docs/src/modules/utils/generateMarkdown.js +++ b/docs/src/modules/utils/generateMarkdown.js @@ -152,7 +152,7 @@ function generatePropDescription(prop) { let notes = ''; if (isElementTypeAcceptingRefProp(type)) { - notes += '
⚠️ [Needs to be able to hold a ref](/guides/composition#caveat-with-refs).'; + notes += '
⚠️ [Needs to be able to hold a ref](/guides/composition/#caveat-with-refs).'; } return `${deprecated}${jsDocText}${signature}${notes}`; diff --git a/pages/api/button-base.md b/pages/api/button-base.md index 0f97e60d0f0cdd..702f2a7bd14cc8 100644 --- a/pages/api/button-base.md +++ b/pages/api/button-base.md @@ -25,7 +25,7 @@ It contains a load of style reset and some focus/ripple logic. | centerRipple | bool | false | If `true`, the ripples will be centered. They won't start at the cursor interaction position. | | children | node | | The content of the component. | | classes | object | | Override or extend the styles applied to the component. See [CSS API](#css) below for more details. | -| component | element type | 'button' | The component used for the root node. Either a string to use a DOM element or a component.
⚠️ [Needs to be able to hold a ref](/guides/composition#caveat-with-refs). | +| component | element type | 'button' | The component used for the root node. Either a string to use a DOM element or a component.
⚠️ [Needs to be able to hold a ref](/guides/composition/#caveat-with-refs). | | disabled | bool | | If `true`, the base button will be disabled. | | disableRipple | bool | false | If `true`, the ripple effect will be disabled. | | disableTouchRipple | bool | false | If `true`, the touch ripple effect will be disabled. | diff --git a/pages/api/click-away-listener.md b/pages/api/click-away-listener.md index 7a25ebe5b7ddb7..8bf7a46d018ce1 100644 --- a/pages/api/click-away-listener.md +++ b/pages/api/click-away-listener.md @@ -19,7 +19,7 @@ For instance, if you need to hide a menu when people click anywhere else on your | Name | Type | Default | Description | |:-----|:-----|:--------|:------------| -| children * | element | | The wrapped element.
⚠️The component used as a child [must be able to hold a ref](/guides/composition#children). | +| children * | element | | The wrapped element.
⚠️The component used as a child [must be able to hold a ref](/guides/composition/#children). | | mouseEvent | enum: 'onClick' |
 'onMouseDown' |
 'onMouseUp' |
 false
 | 'onMouseUp' | The mouse event to listen to. You can disable the listener by providing `false`. | | onClickAway * | func | | Callback fired when a "click away" event is detected. | | touchEvent | enum: 'onTouchStart' |
 'onTouchEnd' |
 false
 | 'onTouchEnd' | The touch event to listen to. You can disable the listener by providing `false`. | diff --git a/pages/api/slide.md b/pages/api/slide.md index 2dd013c6b127f5..a4235a3248b4eb 100644 --- a/pages/api/slide.md +++ b/pages/api/slide.md @@ -19,7 +19,7 @@ It uses [react-transition-group](https://github.com/reactjs/react-transition-gro | Name | Type | Default | Description | |:-----|:-----|:--------|:------------| -| children | element | | A single child content element.
⚠️The component used as a child [must be able to hold a ref](/guides/composition#children). | +| children | element | | A single child content element.
⚠️The component used as a child [must be able to hold a ref](/guides/composition/#children). | | direction | enum: 'left' |
 'right' |
 'up' |
 'down'
 | 'down' | Direction the child node will enter from. | | in | bool | | If `true`, show the component; triggers the enter or exit animation. | | timeout | union: number |
 { enter?: number, exit?: number }
 | { enter: duration.enteringScreen, exit: duration.leavingScreen,} | The duration for the transition, in milliseconds. You may specify a single timeout for all transitions, or individually with an object. |