-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[TS Migration] Improve Form #35584
[TS Migration] Improve Form #35584
Conversation
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
Hey @blazejkustra Could you take a look a this PR too? |
@fabioh8010 I discussed it with @barttom on slack and he prepared another PR on top of it 👍 |
udiff] external = difft
@abdulrahuman5196 Can you please prioritize this issue? It gets a lot of conflicts and type errors with changes on main, if you have any questions. I'm here to assist you 😄 |
Hi, @blazejkustra Sorry, Didn't see the last comment. Will re-review now |
@blazejkustra I did check on high level and have no further comments. But given the PRs size and test range, I would need probability today and tomorrow(hopefully today alone) to check if anything is causing issues. Will on the same. |
Could you kindly fix the lint code meanwhile |
@abdulrahuman5196 Prettier fixed ✅ Thanks for a quick response! |
FYI: Working on this. Will close out sooner. Meanwhile kindly check on conflicts and lint arised(I assume this PR might be prune to more if we leave it out). |
@abdulrahuman5196 I just merged the newest main and adjusted the code after bank refactor 😮💨 |
@blazejkustra I just completed testing all the flows mentioned. Came here to comment the checklist. Seems now I have to test the new bank change as well. 😞 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-02-13.at.11.09.03.PM.mp4Screen.Recording.2024-02-14.at.1.48.37.AM.mp4Screen.Recording.2024-02-14.at.1.56.17.AM.mp4Android: mWeb ChromeScreen.Recording.2024-02-13.at.11.18.34.PM.mp4Screen.Recording.2024-02-14.at.1.58.35.AM.mp4Screen.Recording.2024-02-14.at.2.02.00.AM.mp4iOS: NativeScreen.Recording.2024-02-13.at.10.59.30.PM.mp4Screen.Recording.2024-02-14.at.1.32.07.AM.mp4Screen.Recording.2024-02-14.at.1.36.47.AM.mp4iOS: mWeb SafariScreen.Recording.2024-02-13.at.11.30.16.PM.mp4Screen.Recording.2024-02-14.at.1.40.52.AM.mp4Screen.Recording.2024-02-14.at.1.44.32.AM.mp4MacOS: Chrome / SafariScreen.Recording.2024-02-13.at.9.45.54.PM.mp4Screen.Recording.2024-02-14.at.1.12.15.AM.mp4Screen.Recording.2024-02-14.at.1.16.14.AM.mp4MacOS: DesktopScreen.Recording.2024-02-13.at.10.43.04.PM.mp4Screen.Recording.2024-02-14.at.1.19.41.AM.mp4Screen.Recording.2024-02-14.at.1.28.49.AM.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good and works well. Reviewers checklist is also complete.
All yours. @roryabraham
🎀 👀 🎀
C+ Reviewed
@@ -427,7 +427,7 @@ function connectBankAccountManually(bankAccountID: number, accountNumber?: strin | |||
/** | |||
* Verify the user's identity via Onfido | |||
*/ | |||
function verifyIdentityForBankAccount(bankAccountID: number, onfidoData: OnfidoData) { | |||
function verifyIdentityForBankAccount(bankAccountID: number, onfidoData: Record<string, unknown>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NABish but this seems sus – why are we using Record<string, unknown>
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it but in a different PR, here the type for onfidoData is added
ACCEPT_TERMS: 'acceptTerms', | ||
} as const; | ||
|
||
type AddDebitCardForm = Form<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB / can be done in a follow-up, but there's nothing here to guarantee that the keys of the form type for a given form match the INPUT_IDS
for that form. I can add keys and remove them and there's no errors.
Going to merge this to avoid conflicts. Would love to see follow-ups for the couple concerns listed in my last review |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
[ONYXKEYS.FORMS.NEW_ROOM_FORM]: FormTypes.NewRoomForm; | ||
[ONYXKEYS.FORMS.ROOM_SETTINGS_FORM]: FormTypes.Form; | ||
[ONYXKEYS.FORMS.NEW_TASK_FORM]: FormTypes.Form; | ||
[ONYXKEYS.FORMS.EDIT_TASK_FORM]: FormTypes.Form; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not seeing this earlier, but why are there all these types that are just FormTypes.Form
, even though we have more specific types for these forms, such as the one defined in src/types/form/EditTaskForm
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it was adjusted at some point and I made a mistake while resolving conflicts. I'll fix it in a follow up 👍
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.42-0 🚀
|
@blazejkustra @abdulrahuman5196 can we ignore "Full list of forms with detailed test steps"? |
I think you can test just some of forms, that should be enough |
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.42-5 🚀
|
Details
onPress
is defined to receive a singleGestureResponderEvent
argument, but in reality it can receive different treatment depending on the input that it's used withRecord<string, string>
, but we know that the return type should be a map offormInputID: translationKey
FormValueType
is defined loosely asstring | boolean | Date
, but I'd think that the value type would depend on which form is being referenced.Fixed Issues
$ #35318
PROPOSAL: #32992 (comment)
Tests
src/components/Form/inputs/
, and compare it with what was used in InputWrappers beforeWorkspace new room:
+
button > PressStart chat
> Switch to# Room
-
Room name:
Settings
> PressRoom name
-
Edit room name without change:
Waypoint editor
+
button > Request moneyDistance
tabSave
ValidationStep
011401533
, Account number:1111222233331111
> ContinueAddDebitCardPage:
Full list of forms with detailed test steps in PRs
https://github.com//issues/28878 https://github.com//issues/28879 https://github.com//issues/28877 https://github.com//issues/28876 https://github.com//issues/28875 https://github.com//issues/28874 https://github.com//issues/28873 https://github.com//issues/28872 https://github.com//issues/28871 https://github.com//issues/28870 https://github.com//issues/29997 https://github.com//issues/29998 https://github.com//issues/30000 https://github.com//issues/30002 https://github.com//issues/30003 https://github.com//issues/30004 https://github.com//issues/30006 https://github.com//issues/30008 https://github.com//issues/31559 https://github.com//issues/31562 https://github.com//issues/31566 https://github.com//issues/31565 https://github.com//issues/31612 https://github.com//issues/33356 https://github.com//issues/30305 https://github.com//issues/30306 https://github.com//issues/30307 https://github.com//issues/30308 https://github.com//issues/30310 https://github.com//issues/30311 https://github.com//issues/30312 https://github.com//issues/30314 https://github.com//issues/30316 https://github.com//issues/31561 https://github.com//issues/31563 https://github.com//issues/31564 https://github.com//issues/31560 https://github.com//issues/32064Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android.webm
Android: mWeb Chrome
android-web.webm
iOS: Native
ios.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov