Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Collapse] Warn when collapsedHeight misses units #18435

Closed
popuguytheparrot opened this issue Nov 18, 2019 · 2 comments · Fixed by #18461
Closed

[Collapse] Warn when collapsedHeight misses units #18435

popuguytheparrot opened this issue Nov 18, 2019 · 2 comments · Fixed by #18461
Labels
component: Collapse The React component docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.

Comments

@popuguytheparrot
Copy link

When click on hide after show, the list is not yet collapsed to the desired height

Current Behavior 😯

if click hide to hide height in styles will not change

Expected Behavior 🤔

height remove and sets only min-height in styles

Steps to Reproduce 🕹

https://codesandbox.io/s/keen-thompson-9k8kx

Your Environment 🌎

Tech Version
Material-UI latest
React latest
Browser chrome latest
@popuguytheparrot popuguytheparrot changed the title [Collapse] height dont remove [Collapse] not collapsed back to the desired height Nov 18, 2019
@oliviertassinari oliviertassinari added component: Collapse The React component docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Nov 19, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 19, 2019

@popuguytheparrot Solve the warnings in the console, and you should be good.


However, this surface an opportunity to do better. I would suggest something like this:

diff --git a/packages/material-ui/src/Collapse/Collapse.js b/packages/material-ui/src/Collapse/Collapse.js
index 7295e2045..91300af14 100644
--- a/packages/material-ui/src/Collapse/Collapse.js
+++ b/packages/material-ui/src/Collapse/Collapse.js
@@ -69,6 +69,16 @@ const Collapse = React.forwardRef(function Collapse(props, ref) {

   const handleEnter = (node, isAppearing) => {
     node.style.height = collapsedHeight;
+    if (process.env.NODE_ENV !== 'production') {
+      if (node.style.height !== collapsedHeight) {
+        console.error(
+          [
+            'Material-UI: the `collapsedHeight` prop is invalid.',
+            `You have provided ${collapsedHeight} but it requires units, e.g. '0px'.`,
+          ].join('\n'),
+        );
+      }
+    }

     if (onEnter) {
       onEnter(node, isAppearing);
@@ -138,6 +148,16 @@ const Collapse = React.forwardRef(function Collapse(props, ref) {
     }

     node.style.height = collapsedHeight;
+    if (process.env.NODE_ENV !== 'production') {
+      if (node.style.height !== collapsedHeight) {
+        console.error(
+          [
+            'Material-UI: the `collapsedHeight` prop is invalid.',
+            `You have provided ${collapsedHeight} but it requires units, e.g. '0px'.`,
+          ].join('\n'),
+        );
+      }
+    }

     if (onExiting) {

@oliviertassinari oliviertassinari changed the title [Collapse] not collapsed back to the desired height [Collapse] Warn when collapsedHeight misses units Nov 19, 2019
@popuguytheparrot

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Collapse The React component docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants