-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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] Migrate SearchPage #34655
Merged
yuwenmemon
merged 35 commits into
Expensify:main
from
ruben-rebelo:ts-migration/searchPage
Mar 11, 2024
Merged
Changes from 31 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
5ef38a6
[TS migration] Migrate SearchPage
ruben-rebelo a1bf42a
[TS migration] Added missing key on searchPage OnyxValues
ruben-rebelo 0311d75
[TS migration] SearchPage feedback
ruben-rebelo 043d7a6
[TS migration] SearchPage code improvements
ruben-rebelo 3160959
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 4b868f6
[TS migration] Partially addressed comments
ruben-rebelo 0b6deab
[TS migration] SearchPage minor ts error fix
ruben-rebelo 4885b59
[TS migration] SearchPage Lint
ruben-rebelo 000eb11
[TS Migration] SearchPage route props improvement
ruben-rebelo 221bed7
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo be14da6
[TS migration][SearchPage] Code improvements after merged main
ruben-rebelo 751c974
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo db8c05f
[TS migration] Searchpage navigation prop
ruben-rebelo 0b0ba7a
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 33a370f
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 5802e70
[TS migration][SearchPage] Migrate SearchPage after refactor
ruben-rebelo fe38639
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 8ef09aa
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 15ba310
[TS migration][SearchPage] TS fix
ruben-rebelo 1532321
[TS migration][SearchPage] Typescript adjustements
ruben-rebelo 2cd3918
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo dbaabaf
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo efa4b99
[TS migration][SearchPage] Updated types based on main
ruben-rebelo 4430458
[TS migration][SearchPage] Minor ts issue
ruben-rebelo 5c88c77
Revert "[TS migration][SearchPage] Minor ts issue"
ruben-rebelo 333700b
Revert "[TS migration][SearchPage] Updated types based on main"
ruben-rebelo 75f3770
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 2b01f41
[TS migration][SearchPage] TS issues fix
ruben-rebelo 74061d7
[TS migration][SearchPage] Feedback
ruben-rebelo dcec860
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 44ddd40
[TS migration][SearchPage] TS issues fix
ruben-rebelo 7ba0847
[TS migration][SearchPage] Update src/components/SelectionList/types.ts
ruben-rebelo f6e6552
Merge branch 'main' into ts-migration/searchPage
ruben-rebelo 57fcb34
Merge branch 'ts-migration/searchPage' of https://github.com/ruben-re…
ruben-rebelo bef7c71
[TS migration][SeachPage] Lint
ruben-rebelo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hi, my vacation has just ended. I can review this PR again tomorrow.
And curious, should we support
null
or delete thenull
type inReportutils.OptionData
? I personally prefer the latter, because the style guide says toStrive to type as strictly as possible
, but I'm also open to get more inputs. :)cc @ruben-rebelo, @fabioh8010 , @VickyStash
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.
Well, if
null
is really needed inReportUtils.OptionData
we should support, otherwise let's delete it so we don't need to cascade this?: boolean | null;
up to this component! cc @ruben-rebeloThere 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 agree, I've reverted all the changes made on SelectionList and updated SearchPage.
Please have a look @ntdiary
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.
Yeah, I think you're right. In addition, I'm not sure if using
as
to cast the type is recommended here. It seems that there are no similar cases in the existing migration. 🤔If removing the
null
ofOptionData
is difficult (e.g., big impact range, difficult to test, etc.), then we still have to supportnull
inListItem
first. Type optimization may need to be done in the future.BTW, maybe we can use
MaybePhraseKey
forofflineMessage
, and change thetextInputHint
's type toMaybePhraseKey
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.
@ntdiary I'm afraid with this changed we can't avoid the assertion usage.
Do you prefer to revert this one and keep the nulls for this PR?
I'll update the offlineMessage and textInputHint with these suggestions.
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.
@ruben-rebelo, thank you very much for your attempt, then let's revert to the previous revision and let
ListItem
support thenull
type first. ❤️