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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transition is "appearing" by default and the documentation says that it is false as default #22162

Closed
2 tasks done
italodeandra opened this issue Aug 11, 2020 · 10 comments 路 Fixed by #23221
Closed
2 tasks done
Labels
component: transitions This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/

Comments

@italodeandra
Copy link

  • The issue is present in the latest release.
  • 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
@italodeandra italodeandra added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 11, 2020
@italodeandra
Copy link
Author

italodeandra commented Aug 11, 2020

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

@joshwooding joshwooding added support: question Community support but can be turned into an improvement and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 11, 2020
@oliviertassinari oliviertassinari added component: transitions This is the name of the generic UI component, not the React module! and removed support: question Community support but can be turned into an improvement labels Aug 13, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 13, 2020

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

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Aug 13, 2020
@mbrookes
Copy link
Member

mbrookes commented Aug 14, 2020

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
Copy link
Member

eps1lon commented Aug 14, 2020

@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.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 14, 2020

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: #22162 (comment).

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Aug 14, 2020
@eps1lon
Copy link
Member

eps1lon commented Aug 14, 2020

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.

@fakeharahman
Copy link
Contributor

Hey! Can I take this issue?

@eps1lon
Copy link
Member

eps1lon commented Oct 4, 2020

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.

@mbrookes mbrookes added the hacktoberfest https://hacktoberfest.digitalocean.com/ label Oct 10, 2020
@fakeharahman
Copy link
Contributor

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.

@GuilleDF
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: transitions This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. hacktoberfest https://hacktoberfest.digitalocean.com/
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants