-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Migrate AddPayPalMe to function component #17473
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
41417e4
add useState logic and convert to functional component
NikkiWines 3bd913c
add useRef
NikkiWines 5caf5f5
props instead of this.props
NikkiWines 6307a9e
minor style update
NikkiWines 2d30ed2
cleanup ref logic
NikkiWines 16a9393
Merge branch 'main' of https://github.com/Expensify/App into nikki-mi…
NikkiWines c50a5bb
follow up merge cleanup
NikkiWines 3d5c680
fix props, remove extra space
NikkiWines 88aa2ae
Merge branch 'main' of https://github.com/Expensify/App into nikki-mi…
NikkiWines 19dd2a7
add useCallback, minor style
NikkiWines 66e32cd
update innerRef proptype
NikkiWines 8bb0882
revert unecessarily included changes
NikkiWines b162e3a
include missing dependency
NikkiWines 13ff34e
fix paypal me focus for native apps
NikkiWines 8c789f8
use underscore to fix eslint error
NikkiWines 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
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.
NAB, this kind of made me wonder about whether translations should be memoized. I feel like they aren't now (though we will re-render any components that use
withLocalize()
if the language changes) but maybe it would be better to have auseTranslate('addPayPalMePage.growlMessageOnSave')
that just return the same result unless the language changes. Anyways, not something we should do, but seems like a good use case for hooks.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 was thinking about that when making this change - memoizing translations sounds like a good idea but maybe something to tackle in a larger separate issue?
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.
1000% 👍