[HOLD][No QA] Fix typecheck: remove stale isModalVisible reference#87430
[HOLD][No QA] Fix typecheck: remove stale isModalVisible reference#87430roryabraham wants to merge 1 commit intomainfrom
Conversation
The merge of #86678 (useConfirmModal refactor) left behind a reference to `isModalVisible` that no longer exists after removing the useState hook. The disableAutoFocus prop is no longer needed since useConfirmModal manages modal visibility internally. Made-with: Cursor
|
No C+ review needed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d062d83115
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| break; | ||
| default: | ||
| CurrentStep = <SelectCountryStep disableAutoFocus={isModalVisible} />; | ||
| CurrentStep = <SelectCountryStep />; |
There was a problem hiding this comment.
Keep country search unfocused while exit modal is shown
Removing disableAutoFocus from SelectCountryStep reintroduces the focus regression that fix: delay focus when the modal is visible addressed. In the current flow, PlaidConnectionStep.handlePlaidLinkExit() still triggers onExit and then navigates back, so the country step can mount underneath the confirm modal opened via showConfirmModal; without disableAutoFocus, SelectionList will auto-focus the search input and can pop the keyboard behind the modal on mobile. Please keep a modal-visibility-based autofocus guard for this transition.
Useful? React with 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
Closing in favor of https://github.com/Expensify/App/pull/87431/changes |
Explanation of Change
The merge of #86678 (refactoring
ConfirmModaltouseConfirmModal) left behind a reference toisModalVisibleinAddNewCardPage.tsxafter removing theuseStatehook that defined it. This breaksnpm run typecheckonmain.The
disableAutoFocusprop is no longer needed sinceuseConfirmModalmanages modal visibility internally. This PR simply removes the stale prop.Fixed Issues
$ #87424
Tests
npm run typecheckand verify it passesOffline tests
N/A
QA Steps
n/a
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
N/A — type-only fix, no visual changes.
Made with Cursor