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

[Snackbar] Convert to function component #15504

Merged
merged 6 commits into from Apr 27, 2019

Conversation

adeelibr
Copy link
Contributor

Related to #15231

@joshwooding
Copy link
Member

Don't forget we need to apply this fix: #15231 (comment)

@adeelibr
Copy link
Contributor Author

adeelibr commented Apr 26, 2019

When I try converting the getDerivedStateFromProps method like

-static getDerivedStateFromProps(nextProps, prevState) {
-  if (typeof prevState.exited === 'undefined') {
-    return {
-      exited: !nextProps.open,
-    };
-  }
-
-  if (nextProps.open) {
-    return {
-      exited: false,
-    };
-  }
-
-  return null;
-}

+if (typeof exited === 'undefined') {
+  setExited(!open);
+} else if (open) {
+  setExited(false);
+}

I get an error like;

Too many re-renders. React limits the number of renders to prevent an infinite loop.

I looked into this a bit more 😅 and

if (typeof exited === 'undefined') {
  setExited(!open);
}

is called 7 times during the initial rendering time, & the although setExited(!open) is being called but the value of of exited for those initial 7 renders is always undefined 💭 I am not sure why .. 😅

Reference: https://github.com/mui-org/material-ui/pull/15208/files#r272822227

@adeelibr
Copy link
Contributor Author

Don't forget we need to apply this fix: #15231 (comment)

🌮 💯

@joshwooding
Copy link
Member

Don't forget we need to apply this fix: #15231 (comment)

🌮 💯

That fix means you no longer need getDerivedStateFromProps

@adeelibr
Copy link
Contributor Author

That fix means you no longer need getDerivedStateFromProps

Yes I am looking into it 😄 😄 I didn't see your comment before I posted that long useless comment of mine 🏷 🤣

@adeelibr
Copy link
Contributor Author

@joshwooding Can you kindly review, if you think this implementation is okay, should I move towards the tests?

@joshwooding
Copy link
Member

joshwooding commented Apr 26, 2019

@joshwooding Can you kindly review, if you think this implementation is okay, should I move towards the tests?

Looks good so far. You need to remove the first export though. Haha you already did :P

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 26, 2019

@material-ui/core: parsed: -0.38% 😍, gzip: -0.14% 😍

Details of bundle changes.

Comparing: 72f4f9f...7a65308

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.38% -0.14% 311,943 310,758 84,470 84,352
@material-ui/core/Paper 0.00% -0.02% 67,623 67,623 20,128 20,124
@material-ui/core/Paper.esm 0.00% -0.02% 60,988 60,988 19,024 19,020
@material-ui/core/Popper 0.00% 0.00% 31,114 31,114 10,803 10,803
@material-ui/core/Textarea 0.00% 0.00% 5,472 5,472 2,368 2,368
@material-ui/core/TrapFocus 0.00% 0.00% 3,731 3,731 1,565 1,565
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 15,943 15,943 5,777 5,777
@material-ui/core/useMediaQuery 0.00% 0.00% 2,106 2,106 974 974
@material-ui/lab 0.00% -0.00% 141,033 141,033 42,668 42,667
@material-ui/styles 0.00% -0.05% 51,151 51,151 15,155 15,148
@material-ui/system 0.00% 0.00% 11,765 11,765 3,923 3,923
Button 0.00% -0.01% 85,937 85,937 25,747 25,744
Modal 0.00% -0.08% 20,579 20,579 6,609 6,604
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,154 51,154 11,250 11,250
docs.main -0.19% -0.17% 649,336 648,080 202,481 202,139
packages/material-ui/build/umd/material-ui.production.min.js -0.40% -0.19% 293,659 292,475 82,420 82,263

Generated by 🚫 dangerJS against 7a65308

@joshwooding joshwooding added component: snackbar This is the name of the generic UI component, not the React module! new feature New feature or request labels Apr 27, 2019
@joshwooding joshwooding mentioned this pull request Apr 27, 2019
29 tasks
@oliviertassinari
Copy link
Member

@adeelibr Well done :)

@adeelibr adeelibr deleted the snackbar-hooks-convert branch April 28, 2019 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: snackbar This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants