removed defaultProps and replaced it with a default value where nece… by megs-p · Pull Request #32133 · facebook/react-native

megs-p · 2021-09-01T19:10:00Z

Summary

Removed the deaultProps in the DrawerLayoutAndroid file and replaced it with a default value in case props are undefined.

Changelog

[General] [Changed] - Remove defaultProps from the DrawerLayoutAndroid Component.
@lunaleaps this is the fix for issue #31606

Test Plan

tested on simulator

…ssary

…m as a static field

lunaleaps

Thanks for getting to this! Just the main question about using this.props.drawerBackgroundColor

lunaleaps · 2021-09-01T21:24:11Z

Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js

   * );
   * ```
   */
-  drawerBackgroundColor: ColorValue,


actually we need this in Props i have added it back

We shouldn't remove this prop as users should still be able to specify a backgroundColor other than the default

when we keep this as a part of "Props", it throws an error which says -- """element because property drawerBackgroundColor is missing in
props [1] but exists in Props [2]. [prop-missing]""". Any work around for this? @lunaleaps ? I dont get this error when is do a 'yarn test' locally but from the CI windows-test suite.

lunaleaps · 2021-09-01T21:27:47Z

Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js

          {
            width: this.props.drawerWidth,
-            backgroundColor: this.props.drawerBackgroundColor,
+            backgroundColor: this.props.drawerBackgroundColor || 'white',


Why do we do this? why not use drawerBackgroundColor?

i have removed the const declaration of drawerBackgroundColor above, we can use the || operator at the site of usage, it seems more intuitive.

I'd actually suggest the alternative as other (unlikely) usages of drawerBackgroundColor might come up and not pass 'white' as a default

Sure thanks for the suggestion @lunaleaps ! will go with the earlier approach and not use this.props.drawerBackgroundColor

analysis-bot · 2021-09-01T22:10:39Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 8c25711

…rawerBackgroundColor

lunaleaps

lgtm

facebook-github-bot · 2021-09-02T06:15:41Z

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cortinico · 2021-09-02T09:44:14Z

Hey @mdaj06,
Don't you mind rebasing your branch on top of main. A lot of the CI failures have been fixed and seems like you don't have the fixes in your branch yet

…outAndroid

megs-p · 2021-09-02T10:09:42Z

Hi @cortinico looks like my fork was 80 commits behind, i have now fetched upstream and have merged those commits from main to my branch. This should work i guess please correct me if i am wrong.Thanks!

cortinico · 2021-09-02T10:22:55Z

Some of the CI failures are valid:


Cannot create `AndroidDrawerLayoutNativeComponent` element because property `drawerBackgroundColor` is missing in
props [1] but exists in `NativeProps` [2]. [prop-missing]

   Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js:216:8
   216|       <AndroidDrawerLayoutNativeComponent
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can I ask you to take a look at them? Let us know if you get stuck 👍

megs-p · 2021-09-02T10:24:01Z

@cortinico okay sure will have a look at this and get back to you!

analysis-bot · 2021-09-02T10:41:18Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,957,335	-13
android	hermes	armeabi-v7a	8,490,628	+1
android	hermes	x86	9,377,554	-13
android	hermes	x86_64	9,342,875	-12
android	jsc	arm64-v8a	10,639,601	+9
android	jsc	armeabi-v7a	9,560,403	+13
android	jsc	x86	10,654,101	+11
android	jsc	x86_64	11,263,105	+14

Base commit: 8c25711

…ponent since the native component expects it

…as a default inside render()

analysis-bot

Code analysis results:

eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js

megs-p · 2021-09-02T11:40:15Z

@cortinico I have removed the drawerBackgroundColor from Props as we are giving it a default value later on. Also have added a prop seperately to the AndroidDrawerLayoutNativeComponent such that the native component can run as expected. Does that look good to you?
EDIT:i have updated the snapshot test as well

…aultProps

…aultProps drawerBackgroundColor

cortinico

Does that look good to you?

It does 👍
Please remove the gradlew file change that should not be included.
Handing over to @lunaleaps for merging this

cortinico · 2021-09-02T16:38:17Z

gradlew.bat

@@ -1,89 +1,89 @@
-@rem


Do not add this file (valid for all the gradlew files).

oh yes must have got in there during my earlier commit! sorry! will remove it asap

Just to clarify: you don't need to remove the file, but just to amend the changes 👍

As @cortinico mentioned, we shouldn't be deleting these files, just undo your changes.. and similarly for the other gradlew.bat files. We probably shouldn't be touching those. @cortinico to verify

sure will reinstatie them

facebook-github-bot · 2021-09-06T19:40:47Z

@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

lunaleaps · 2021-09-06T19:46:53Z

Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js

   * );
   * ```
   */
-  drawerBackgroundColor: ColorValue,


We shouldn't remove this prop as users should still be able to specify a backgroundColor other than the default

lunaleaps · 2021-09-06T19:53:39Z

gradlew.bat

@@ -1,89 +1,89 @@
-@rem


As @cortinico mentioned, we shouldn't be deleting these files, just undo your changes.. and similarly for the other gradlew.bat files. We probably shouldn't be touching those. @cortinico to verify

…ture/reafctorDrawerLayoutAndroidV2

Feature/reafctor drawer layout android v2

pull-bot · 2021-09-06T21:32:28Z

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against 66384ef

megs-p · 2021-09-06T21:35:39Z

closed this PR as it got too messy will reopen one @lunaleaps with the suggested changes.

removed defaultProps and replaced it with a default value where nece…

f0f73e6

…ssary

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 1, 2021

mdaj06 added 5 commits September 2, 2021 00:47

removed drawerBackgroundColor from Props since we have removed it fro…

75d45c8

…m as a static field

updated the props list with a default value

6a9a2cd

reinstated the prop value

fe133f6

deleted the Prop value instead of commenting it

b2e7f7f

added drawerBackgroundColor to type prop

b535543

lunaleaps self-assigned this Sep 1, 2021

lunaleaps requested changes Sep 1, 2021

View reviewed changes

lunaleaps added the Needs: Author Feedback label Sep 1, 2021

removed background colour initialization

ceaeb39

removed usage of this.props.drawerBackgroundColor and added a const d…

a9e3153

…rawerBackgroundColor

lunaleaps approved these changes Sep 2, 2021

View reviewed changes

Merge branch 'facebook:main' into feature/removeDefaultPropsDrawerLay…

d895d71

…outAndroid

github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Sep 2, 2021

mdaj06 added 2 commits September 2, 2021 16:29

added drawerBackgroundColor as a prop to AndroidDrawerLayoutNativeCom…

c1e1655

…ponent since the native component expects it

removed the drawerbackgroundColor from Props since we are setting it …

4f89ce0

…as a default inside render()

analysis-bot suggested changes Sep 2, 2021

View reviewed changes

Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js Outdated Show resolved Hide resolved

Libraries/Components/DrawerAndroid/DrawerLayoutAndroid.android.js Outdated Show resolved Hide resolved

refactored to follow eslint rules as suggested by the code analyzer

7e077b2

mdaj06 added 2 commits September 2, 2021 18:33

updated snapshots tests for DrawerAndroid to match the removal of def…

6514b67

…aultProps

updated snapshots tests for DrawerAndroid to match the removal of def…

0252eb3

…aultProps drawerBackgroundColor

megs-p requested a review from lunaleaps September 2, 2021 14:38

cortinico approved these changes Sep 2, 2021

View reviewed changes

megs-p added 3 commits September 2, 2021 22:16

Delete gradlew.bat

8145cea

Delete gradlew.bat

40a6c11

Delete gradlew.bat

8767dff

lunaleaps requested changes Sep 6, 2021

View reviewed changes

mdaj06 and others added 4 commits September 7, 2021 02:06

remerged main to get all the upstream changes

2daa3a6

reinstated the bat files and added back the property to Props

d889996

Merge branch 'feature/removeDefaultPropsDrawerLayoutAndroid' into fea…

acd6196

…ture/reafctorDrawerLayoutAndroidV2

Merge pull request #1 from mdaj06/feature/reafctorDrawerLayoutAndroidV2

66384ef

Feature/reafctor drawer layout android v2

megs-p requested review from hramos and mhorowitz as code owners September 6, 2021 21:23

megs-p closed this Sep 6, 2021

cortinico mentioned this pull request Sep 7, 2021

Fix/remove default props/android layout drawer #32160

Closed

Conversation

megs-p commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changelog

Test Plan

Uh oh!

lunaleaps left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megs-p Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megs-p Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megs-p Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

analysis-bot commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunaleaps left a comment

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 2, 2021

Uh oh!

cortinico commented Sep 2, 2021

Uh oh!

megs-p commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortinico commented Sep 2, 2021

Uh oh!

megs-p commented Sep 2, 2021

Uh oh!

analysis-bot commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

analysis-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

megs-p commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortinico left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Sep 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

megs-p commented Sep 1, 2021 •

edited

Loading

lunaleaps left a comment •

edited

Loading

megs-p Sep 6, 2021 •

edited

Loading

megs-p Sep 1, 2021 •

edited

Loading

megs-p Sep 2, 2021 •

edited

Loading

analysis-bot commented Sep 1, 2021 •

edited

Loading

megs-p commented Sep 2, 2021 •

edited

Loading

analysis-bot commented Sep 2, 2021 •

edited

Loading

megs-p commented Sep 2, 2021 •

edited

Loading