[ButtonBase] Fix when changing enableRipple prop from false to true (m…

…ui#19667)
EsoterikStare · Mar 30, 2020 · a54f6c6 · a54f6c6
1 parent 60b3f73
commit a54f6c6
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 8 deletions.
diff --git a/docs/pages/api/no-ssr.md b/docs/pages/api/no-ssr.md
@@ -30,7 +30,7 @@ This component can be useful in a variety of situations:
 
 | Name | Type | Default | Description |
 |:-----|:-----|:--------|:------------|
-| <span class="prop-name required">children&nbsp;*</span> | <span class="prop-type">node</span> |  | You can wrap a node. |
+| <span class="prop-name">children</span> | <span class="prop-type">node</span> |  | You can wrap a node. |
 | <span class="prop-name">defer</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the component will not only prevent server-side rendering. It will also defer the rendering of the children into a different screen frame. |
 | <span class="prop-name">fallback</span> | <span class="prop-type">node</span> | <span class="prop-default">null</span> | The fallback content to display. |
 

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.js b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -304,12 +304,12 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) {
       {...other}
     >
       {children}
-      {!disableRipple && !disabled ? (
-        <NoSsr>
-          {/* TouchRipple is only needed client-side, x2 boost on the server. */}
+      <NoSsr>
+        {!disableRipple && !disabled ? (
+          /* TouchRipple is only needed client-side, x2 boost on the server. */
           <TouchRipple ref={rippleRef} center={centerRipple} {...TouchRippleProps} />
-        </NoSsr>
-      ) : null}
+        ) : null}
+      </NoSsr>
     </ComponentProp>
   );
 });

diff --git a/packages/material-ui/src/ButtonBase/ButtonBase.test.js b/packages/material-ui/src/ButtonBase/ButtonBase.test.js
@@ -374,6 +374,54 @@ describe('<ButtonBase />', () => {
           button.querySelectorAll('.ripple-visible .child:not(.child-leaving)'),
         ).to.have.lengthOf(0);
       });
+
+      it('should not crash when changes enableRipple from false to true', () => {
+        function App() {
+          /** @type {React.MutableRefObject<import('./ButtonBase').ButtonBaseActions | null>} */
+          const buttonRef = React.useRef(null);
+          const [enableRipple, setRipple] = React.useState(false);
+
+          React.useEffect(() => {
+            if (buttonRef.current) {
+              buttonRef.current.focusVisible();
+            } else {
+              throw new Error('buttonRef.current must be available');
+            }
+          }, []);
+
+          return (
+            <div>
+              <button
+                type="button"
+                data-testid="trigger"
+                onClick={() => {
+                  setRipple(true);
+                }}
+              >
+                Trigger crash
+              </button>
+              <ButtonBase
+                autoFocus
+                action={buttonRef}
+                TouchRippleProps={{
+                  classes: {
+                    ripplePulsate: 'ripple-pulsate',
+                  },
+                }}
+                focusRipple
+                disableRipple={!enableRipple}
+              >
+                the button
+              </ButtonBase>
+            </div>
+          );
+        }
+
+        const { container, getByTestId } = render(<App />);
+
+        fireEvent.click(getByTestId('trigger'));
+        expect(container.querySelectorAll('.ripple-pulsate')).to.have.lengthOf(1);
+      });
     });
   });
 

diff --git a/packages/material-ui/src/NoSsr/NoSsr.d.ts b/packages/material-ui/src/NoSsr/NoSsr.d.ts
@@ -1,7 +1,7 @@
 import * as React from 'react';
 
 export interface NoSsrProps {
-  children: React.ReactNode;
+  children?: React.ReactNode;
   defer?: boolean;
   fallback?: React.ReactNode;
 }

diff --git a/packages/material-ui/src/NoSsr/NoSsr.js b/packages/material-ui/src/NoSsr/NoSsr.js
@@ -40,7 +40,7 @@ NoSsr.propTypes = {
   /**
    * You can wrap a node.
    */
-  children: PropTypes.node.isRequired,
+  children: PropTypes.node,
   /**
    * If `true`, the component will not only prevent server-side rendering.
    * It will also defer the rendering of the children into a different screen frame.