refactor checkbox with label by thesahindia · Pull Request #7750 · Expensify/App

thesahindia · 2022-02-14T22:36:46Z

Details

Fixed Issues

$ #7531

Tests

Run npm run storybook
Try submitting the form
Check if shows error

Verify that no errors appear in the JS console

QA Steps

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

Screen.Recording.2022-02-15.at.2.34.31.AM.mov

without margin left-

with margin left-

Screen.Recording.2022-02-15.at.2.37.02.AM.mov

Mobile Web

Desktop

iOS

Android

thesahindia · 2022-02-14T22:38:33Z

Still having issues with desktop app. Will try again after sometime.

rushatgabhane · 2022-02-14T22:39:28Z

@thesahindia to test on desktop, merge main from upstream into your branch.

And temporarily create a .env file with USE_WEB_PROXY= true and USE_NRGOK=false

thesahindia · 2022-02-14T22:40:56Z

@rushatgabhane, tried both but still it's not showing anything.

src/components/Checkbox.js

src/stories/Form.stories.js

rushatgabhane · 2022-02-15T18:39:29Z

@thesahindia i can't toggle the checkbox. Maybe it's because of regression in TextInput #7318 (comment)

thesahindia · 2022-02-16T21:44:32Z

@rushatgabhane, hadn't implemented that because of getting error while testing due to the regression. It should work now.

src/components/CheckboxWithLabel.js

rushatgabhane · 2022-02-18T15:49:17Z

hadn't implemented that because of getting error while testing due to the regression. It should work now.

Nope, it doesn't work. I'm unable to toggle checkbox in storybook.
Are you sure it's not due to the regression in TextInput #7318 (comment)

thesahindia · 2022-02-18T16:26:01Z

Nope, it doesn't work. I'm unable to toggle checkbox in storybook. Are you sure it's not due to the regression in TextInput #7318 (comment)

Yeah I think it might be due to the regression as I had tested and it was working fine.

tgolen

Looks like this is still needing desktop testing.

src/components/Checkbox.js

src/components/CheckboxWithLabel.js

rushatgabhane · 2022-02-24T20:13:12Z

@thesahindia please merge main before you work on the changes requested. Thanks so much

thesahindia · 2022-02-24T20:36:53Z

@thesahindia please merge main before you work on the changes requested. Thanks so much

@rushatgabhane, sure and sorry for the delay. I was waiting to get clarification about adding translation but didn't see it was resolved (don't know why I didn't get a notification for it 🤷‍♂️)

src/components/Checkbox.js

…factor-checkbox-with-label merge main

src/components/CheckboxWithLabel.js

rushatgabhane

@tgolen LGTM! 🎉

rushatgabhane · 2022-03-09T16:40:34Z

tgolen

Just noticed this one last thing before merging.

tgolen · 2022-03-09T17:23:40Z

src/components/Checkbox.js

@@ -13,27 +13,35 @@ const propTypes = {
    onPress: PropTypes.func.isRequired,

    /** Should the input be styled for errors  */


Looks like this comment is out-of-date

Earlier we were applying the error styles when hasError was true, now we are just checking if errorText is present, so the functionality hasn't change and I think we can keep this comment.

@thesahindia "Should the input be styled for errors" gives the impression that it's a boolean.
And we're displaying whatever error is passed.

Suggested change

/** Should the input be styled for errors */

/** Error text to display */

@rushatgabhane, Yes makes sense, but now I think a better way would be to keep it as hasError since we are only going to use it as boolean and I will pass it like this -

<Checkbox hasError={!!props.errorText}

the errorText that we get in CheckboxWithLabel is to show the error message and for checkbox component we only need a boolean value

@thesahindia good call. I agree, let's do that.

hasError={Boolean(props.errorText)}

Done ✔️

@tgolen all yours! LGTM

tgolen

Thanks for that change. I love the removal of hasError!

OSBotify · 2022-03-10T17:54:07Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

rushatgabhane · 2022-03-10T21:29:22Z

⚠️ @thesahindia we have an issue. getInputIDPropTypes needs to be renamed to validateInputIDProps

Please create a follow-up PR.

Edit: I've created a PR to fix this because it's 3 AM for you

OSBotify · 2022-03-11T00:59:17Z

🚀 Deployed to staging by @tgolen in version: 1.1.42-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

thesahindia · 2022-03-11T07:36:43Z

Edit: I've created a PR to fix this because it's 3 AM for you

Thanks for the help 😃

OSBotify · 2022-03-15T20:52:00Z

🚀 Deployed to production by @chiragsalian in version: 1.1.42-6 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

thesahindia added 2 commits February 15, 2022 03:45

refactor checkboxWithLabel

3acceaa

add checkboxWithLabel to form stories

2589aea

thesahindia requested a review from a team as a code owner February 14, 2022 22:36

MelvinBot requested review from rushatgabhane and tgolen and removed request for a team February 14, 2022 22:36

tgolen requested changes Feb 15, 2022

View reviewed changes

src/components/Checkbox.js Show resolved Hide resolved

src/stories/Form.stories.js Show resolved Hide resolved

src/stories/Form.stories.js Show resolved Hide resolved

improvement

143d19a

rushatgabhane reviewed Feb 18, 2022

View reviewed changes

src/components/CheckboxWithLabel.js Outdated Show resolved Hide resolved

tgolen requested changes Feb 22, 2022

View reviewed changes

src/components/Checkbox.js Show resolved Hide resolved

src/components/CheckboxWithLabel.js Outdated Show resolved Hide resolved

parasharrajat reviewed Feb 24, 2022

View reviewed changes

src/components/Checkbox.js Outdated Show resolved Hide resolved

thesahindia added 5 commits February 25, 2022 02:50

Merge branch 'main' of github.com:thesahindia/App into thesahindia/re…

a19455c

…factor-checkbox-with-label merge main

create function for onPress

1b9605f

change type

046fdcd

merge main

3ab266a

remove extra line

801fa9c

rushatgabhane suggested changes Mar 2, 2022

View reviewed changes

src/components/CheckboxWithLabel.js Outdated Show resolved Hide resolved

pass onCheckboxPress in TouchableOpacity

714d1e5

rushatgabhane reviewed Mar 2, 2022

View reviewed changes

src/components/CheckboxWithLabel.js Outdated Show resolved Hide resolved

tgolen requested changes Mar 3, 2022

View reviewed changes

src/components/CheckboxWithLabel.js Outdated Show resolved Hide resolved

merge main

2878eb3

change function name

963593a

rushatgabhane reviewed Mar 7, 2022

View reviewed changes

src/components/CheckboxWithLabel.js Show resolved Hide resolved

rushatgabhane previously approved these changes Mar 9, 2022

View reviewed changes

tgolen requested changes Mar 9, 2022

View reviewed changes

use hasError

32df660

thesahindia dismissed rushatgabhane’s stale review via 32df660 March 9, 2022 22:45

tgolen approved these changes Mar 10, 2022

View reviewed changes

tgolen merged commit f0a4118 into Expensify:main Mar 10, 2022

rushatgabhane mentioned this pull request Mar 10, 2022

[No QA] Rename getInputIDPropTypes to validateInputIDProps #8069

Merged

50 tasks

OSBotify mentioned this pull request Mar 10, 2022

Deploy Checklist: New Expensify 2022-03-10 #8074

Closed

botify mentioned this pull request Mar 15, 2022

[HOLD for payment 2022-03-22] Refactor CheckboxWithLabel to be compatible with Form #7531

Closed

luacmartins mentioned this pull request Apr 18, 2022

Clean up CheckboxWithLabel refactor #8679

Closed

		@@ -13,27 +13,35 @@ const propTypes = {
		onPress: PropTypes.func.isRequired,

		/** Should the input be styled for errors */

	/** Should the input be styled for errors */
	/** Error text to display */

Conversation

thesahindia commented Feb 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

thesahindia commented Feb 14, 2022

Uh oh!

rushatgabhane commented Feb 14, 2022

Uh oh!

thesahindia commented Feb 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rushatgabhane commented Feb 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thesahindia commented Feb 16, 2022

Uh oh!

Uh oh!

rushatgabhane commented Feb 18, 2022

Uh oh!

thesahindia commented Feb 18, 2022

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rushatgabhane commented Feb 24, 2022

Uh oh!

thesahindia commented Feb 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rushatgabhane left a comment

Choose a reason for hiding this comment

Uh oh!

rushatgabhane commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Checklist

Uh oh!

tgolen left a comment

Choose a reason for hiding this comment

Uh oh!

tgolen Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

thesahindia Mar 9, 2022 • 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!

rushatgabhane Mar 9, 2022 • 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!

thesahindia Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

rushatgabhane Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

thesahindia Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

rushatgabhane Mar 9, 2022

Choose a reason for hiding this comment

thesahindia commented Feb 14, 2022 •

edited

Loading

rushatgabhane commented Feb 15, 2022 •

edited

Loading

rushatgabhane commented Mar 9, 2022 •

edited

Loading

thesahindia Mar 9, 2022 •

edited

Loading

rushatgabhane Mar 9, 2022 •

edited

Loading

rushatgabhane commented Mar 10, 2022 •

edited

Loading