16185 - Migrate PDFPasswordForm.js to function component#24290
Merged
marcaaron merged 9 commits intoExpensify:mainfrom Aug 17, 2023
Merged
16185 - Migrate PDFPasswordForm.js to function component#24290marcaaron merged 9 commits intoExpensify:mainfrom
marcaaron merged 9 commits intoExpensify:mainfrom
Conversation
s77rt
suggested changes
Aug 9, 2023
| const textInputRef = useRef(null); | ||
|
|
||
| useEffect(() => { | ||
| if (!props.isFocused || !textInputRef.current) return; |
Member
There was a problem hiding this comment.
Suggested change
| if (!props.isFocused || !textInputRef.current) return; | |
| if (!props.isFocused) { | |
| return; | |
| } | |
| if (!textInputRef.current) { | |
| return; | |
| } |
NAB I think it's more readable
| } | ||
| this.textInputRef.focus(); | ||
| } | ||
| function PDFPasswordForm(props) { |
| textInputRef.current.focus(); | ||
| }, [props.isFocused]); | ||
|
|
||
| const getErrorText = () => { |
Member
There was a problem hiding this comment.
Can we define the errorText without defining the function since this function is only used once.
| submitPassword() { | ||
| if (!this.validate()) { | ||
| return; | ||
| const updatePassword = (passwrd) => { |
Member
There was a problem hiding this comment.
Suggested change
| const updatePassword = (passwrd) => { | |
| const updatePassword = (newPassword) => { |
NAB passwrd looks like a typo, it may be confusing with password. newPassword looks more as an intended change.
Comment on lines
101
to
103
| const showForm = () => { | ||
| setShouldShowForm(true); | ||
| }; |
Member
There was a problem hiding this comment.
Inline this in PDFInfoMessage's onShowForm
| const containerStyle = isSmallScreenWidth ? [styles.flex1, styles.w100] : styles.pdfPasswordForm.wideScreenWidth; | ||
|
|
||
| return ( | ||
| <> |
Member
There was a problem hiding this comment.
This is not resolved yet. Remove the extra <> and </> (then npm run prettier)
s77rt
suggested changes
Aug 10, 2023
| const containerStyle = isSmallScreenWidth ? [styles.flex1, styles.w100] : styles.pdfPasswordForm.wideScreenWidth; | ||
|
|
||
| return ( | ||
| <> |
Member
There was a problem hiding this comment.
This is not resolved yet. Remove the extra <> and </> (then npm run prettier)
| onPasswordFieldFocused(false); | ||
| }; | ||
|
|
||
| const showForm = () => setShouldShowForm(true); |
Member
There was a problem hiding this comment.
What I meant is to pass the function directly to PDFInfoMessage without defining it.
<PDFInfoMessage onShowForm={() => setShouldShowForm(true)} />| {shouldShowForm ? ( | ||
| <ScrollView | ||
| keyboardShouldPersistTaps="handled" | ||
| style={isSmallScreenWidth ? [styles.flex1, styles.w100] : styles.pdfPasswordForm.wideScreenWidth} |
Member
There was a problem hiding this comment.
Suggested change
| style={isSmallScreenWidth ? [styles.flex1, styles.w100] : styles.pdfPasswordForm.wideScreenWidth} | |
| style={styles.pdfPasswordForm(isSmallScreenWidth)} |
And in styles.js
pdfPasswordForm: (isSmallScreenWidth) => ({
width: isSmallScreenWidth ? '100%' : 350,
...(isSmallScreenWidth && flex.flex1),
}),
Member
Reviewer Checklist
Screenshots/VideosWebweb.mp4Mobile Web - Chromemweb-chrome.mp4Mobile Web - Safarimweb-safari.mp4Desktopdesktop.mp4iOSios.mp4Androidandroid.mp4 |
Member
|
@Skalakid I'm having trouble testing this in Native. I'm not sure if this is related to this PR or something from my end, can you please double check this is working well from your side? I'm getting an error
|
Member
|
It looks that we may have another problem in main https://expensify.slack.com/archives/C01GTK53T8Q/p1691851369441659 that is blocking testing. |
Contributor
Author
marcaaron
requested changes
Aug 14, 2023
Contributor
marcaaron
left a comment
marcaaron
left a comment
There was a problem hiding this comment.
Looks good just a couple of small comments.
| return shouldShowForm ? ( | ||
| <ScrollView | ||
| keyboardShouldPersistTaps="handled" | ||
| style={styles.pdfPasswordForm(isSmallScreenWidth)} |
Contributor
There was a problem hiding this comment.
This should be something like getPDFPasswordFormStyle(). Functions usually are named for what they do or return.
|
|
||
| return ''; | ||
| } | ||
| })(); |
Contributor
There was a problem hiding this comment.
Maybe we can just do a useMemo() here if we are going to use an IIFE.
marcaaron
approved these changes
Aug 17, 2023
Contributor
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
1 task
Contributor
|
🚀 Deployed to staging by https://github.com/marcaaron in version: 1.3.56-0 🚀
|
Contributor
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.56-24 🚀
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Details
Fixed Issues
$ #16185
PROPOSAL: #16185
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
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
web.mov
Mobile Web - Chrome
mWeb-chrome.mov
Mobile Web - Safari
mWeb-safari.mov
Desktop
desktop.mov
iOS
ios.mov
Android
andorid.mov