Migrate DatePicker/index.android.js to function component #16136 #25657
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
arthurmfgtab
changed the title
arthurmfgtab
changed the title
…omp-to-functional
| <RNDatePicker | ||
| value={value || defaultValue ? moment(value || defaultValue).toDate() : new Date()} | ||
| mode="date" | ||
| onChange={() => setDate()} |
There was a problem hiding this comment.
Did you test android app after your last commit? setDate requires 2 params.
Please add screenshot, at least for android.
There was a problem hiding this comment.
Oh, so sorry about that :/
Just fixed it and attached a evidence for Android
| errorText={errorText} | ||
| containerStyles={containerStyles} | ||
| textInputContainerStyles={isPickerVisible ? [styles.borderColorFocus] : []} | ||
| onPress={() => showPicker()} |
There was a problem hiding this comment.
This works for arrow functions. Maybe you had used function keyword before.
| onPress={() => showPicker()} | |
| onPress={showPicker} |
| editable={false} | ||
| disabled={disabled} | ||
| onBlur={onBlur} | ||
| ref={(element) => setRef(element)} |
There was a problem hiding this comment.
Same here:
| ref={(element) => setRef(element)} | |
| ref={setRef} |
| <RNDatePicker | ||
| value={value || defaultValue ? moment(value || defaultValue).toDate() : new Date()} | ||
| mode="date" | ||
| onChange={() => setDate()} |
There was a problem hiding this comment.
| onChange={() => setDate()} | |
| onChange={setDate} |
| import {Keyboard} from 'react-native'; | ||
| import RNDatePicker from '@react-native-community/datetimepicker'; | ||
| import moment from 'moment'; | ||
| import _ from 'underscore'; | ||
| import isFunction from 'lodash/isFunction'; |
There was a problem hiding this comment.
Any reason for replacing underscore with lodash?
There was a problem hiding this comment.
It was suggested by a review from our team, but I just reverted it back to underscore
There was a problem hiding this comment.
@situchan, AFAIK, there is a reason - #27459 (comment).
cc @arthurmfgtab
There was a problem hiding this comment.
Gotcha. But we haven't ever used lodash/isFunction pattern in our app. Just don't wanna be the first introducer.
There was a problem hiding this comment.
Got it. It is up to you guys, I just remember my conversation about it.
|
There's unverified commit |
Gentle bump ^ 
|
|
@situchan after having a couple of issues with conflicts while trying to sign an old commit I decided to created another PR and ditch this one, here's the one I also fixed my git config so now every commit will be properly signed automatically, that way I won't have this signing issues again. |
Details
Fixed Issues
$ #16136
PROPOSAL: N/A
Tests
Offline tests
Not applicable.
QA Steps
Not applicable.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
Screen.Recording.2023-09-21.at.02.55.18.mov