Material-ui: Transition is "appearing" by default and the documentation says that it is false as default

Created on 11 Aug 2020  路  10Comments  路  Source: mui-org/material-ui

  • [x] The issue is present in the latest release.
  • [x] I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 馃槸

When using the component Transition, the content "appears" instead of having the default behavior of not appearing on the first mount.

Expected Behavior 馃

The component should not appear on the first mount, as stated on the documentation.

Steps to Reproduce 馃暪

Steps:

  1. Use any Transition component without "appear" property.

https://codesandbox.io/s/nice-hooks-fpi84?file=/src/Demo.js

Context 馃敠

Your Environment 馃寧

| Tech | Version |
| ----------- | ------- |
| Material-UI | v4.11.0 |
| React | 16.13.1 |
| Browser | Google Chrome 84.0.4147.125 (Official Build) (64-bit) |
| TypeScript | 3.9.7 |

Transition docs good first issue hacktoberfest

Most helpful comment

Maybe we could have a new prop disableAppear to have the default as false and avoid the change of behavior.

I think we can make an argument the documenting appear as a default implies that <Grow appear={undefined} /> leads to <Transition appear={true} />. Let's just call it a bug (appear should default to true; previously omitting appear was treated differently than setting appear to undefined) and schedule it for v5 to reduce risk of unwanted breakage.

All 10 comments

I just noticed that here in the Material UI the appear is actually default to true. But why?

https://github.com/mui-org/material-ui/blob/afa8a586b46a759bb3a416b634bad657664b8ad7/packages/material-ui/src/Fade/Fade.js#L107-L109

This problem seems to be another #11683 or #21924 more recently. We have a prop that is inherited from somewhere else, that we don't document, but yet, we change its default value.

Displaying the props with its custom default prop as with the others could be our "best worse" option. Any better UX in mind? (without considerations for the implementation)

@eps1lon What solution do you have in mind with? What will be the UX?

our API docs will get a major overhaul

But why?

I'm never going to find it, but I do vaguely recall a discussion back when the transitions were added as to what would be the most sane default. @oliviertassinari is right though - it should be documented.

@eps1lon What solution do you have in mind with? What will be the UX?

The way this default is setup is different to any pattern we're currently using.

diff --git a/packages/material-ui/src/Grow/Grow.js b/packages/material-ui/src/Grow/Grow.js
index 6ab04f846d..f3b01095c9 100644
--- a/packages/material-ui/src/Grow/Grow.js
+++ b/packages/material-ui/src/Grow/Grow.js
@@ -28,6 +28,7 @@ const styles = {
  */
 const Grow = React.forwardRef(function Grow(props, ref) {
   const {
+    appear = true,
     children,
     in: inProp,
     onEnter,
@@ -154,7 +155,7 @@ const Grow = React.forwardRef(function Grow(props, ref) {

   return (
     <TransitionComponent
-      appear
+      appear={appear}
       in={inProp}
       nodeRef={nodeRef}
       onEnter={handleEnter}

is the smallest possible change right now (+ yarn proptypes && yarn docs:api) to add documentation. Just a word of caution: This is backwards incompatible if you had <Grow appear={undefined} /> which would lead to <Transition appear={false} /> but is now <Transition appear={true} />. Probably very unlikely to break but could be possible when spreading props around. We can use v5 for that.

Though I don't think we ever had this default documented.

Though I don't think we ever had this default documented.

@eps1lon As far as I know, we never had it documented. Maybe we could have a new prop disableAppear to have the default as false and avoid the change of behavior.

Hum, maybe not. I'm all 馃挴 for the proposed changes: https://github.com/mui-org/material-ui/issues/22162#issuecomment-673984372.

Maybe we could have a new prop disableAppear to have the default as false and avoid the change of behavior.

I think we can make an argument the documenting appear as a default implies that <Grow appear={undefined} /> leads to <Transition appear={true} />. Let's just call it a bug (appear should default to true; previously omitting appear was treated differently than setting appear to undefined) and schedule it for v5 to reduce risk of unwanted breakage.

Hey! Can I take this issue?

Hey! Can I take this issue?

@fakeharahman All yours. Please let us know if you no longer intend to work on this issue. If you're stuck at any point feel free to open a PR so a maintainer can take a look.

Hey, I got stuck on this so if someone else wants to take this, they are the most welcome to! I will still try to solve this.

Hi! I can take this if it's okay! 馃槃

Was this page helpful?
0 / 5 - 0 ratings