-
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
updated to function component #28625
updated to function component #28625
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
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.
LGTM, after resolving comments.
I have read the CLA Document and I hereby sign the CLA |
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.
Vit asked me to review once PR is up, I left some notes from the previous PR that I worked on.
@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@eVoloshchak I have merged the newest main and resolved conflicts. I did also fix the linter error and warnings. |
@keisyrzk, the order of imports should be different, the Lint check will still fail |
@eVoloshchak ran prettier and linter again and pushed updates |
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.
SpinningIndicatorAnimation
isn't deleted anymore, must have been a mismatch during merge
@eVoloshchak ok I have added the |
@keisyrzk @eVoloshchak Why |
@keisyrzk, I meant we need to delete the |
…and resolve the merge conflict 😬 |
@eVoloshchak oh ok, I misunderstood your comment. I have removed the |
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.
Co-authored-by: Eugene Voloshchak <copyreading@gmail.com>
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.
Crash on Android
Screen.Recording.2023-11-12.at.14.28.23.mov
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2023-11-21.at.13.14.48.movAndroid: mWeb ChromeScreen.Recording.2023-11-21.at.13.17.12.moviOS: NativeScreen.Recording.2023-11-21.at.13.24.27.moviOS: mWeb SafariScreen.Recording.2023-11-21.at.13.30.09.movMacOS: Chrome / SafariScreen.Recording.2023-11-20.at.17.47.08.movMacOS: DesktopScreen.Recording.2023-11-21.at.13.32.14.mov |
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.
LGTM
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.
LGTM2! Nice to see this one get over the finish line. Thanks for the hard work @keisyrzk & @eVoloshchak!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/deetergp in version: 1.4.2-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.4.2-3 🚀
|
Details
Updated the component so it is now built with a
function
.Fixed Issues
$ #16119
Tests
4a. click every row in the list and go back
4b. click on the avatar so the bottom sheet appears
6a. click on the screen outside the sheet - check if closes without issues
6b. click on the "gallery" option
10a. click on the screen outside the sheet - check if closes without issues
10b. click "view photo" and then go back
10c. click "upload photo" and repeat the proccess of picking a photo
10d. click "remove photo" - see if avatar changed to default one
Offline tests
NA
QA Steps
The same as described in "Tests" section.
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(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
https://github.com/Expensify/App/assets/18525360/63ad1dc3-6658-4ea3-b1ce-f1d9dc2d89ee https://github.com/Expensify/App/assets/18525360/3c79afc2-beff-4b3c-9211-c5e03389727dMobile Web - Chrome
https://github.com/Expensify/App/assets/18525360/191935fc-244a-4786-a5f5-60a8f83002bcMobile Web - Safari
https://github.com/Expensify/App/assets/18525360/fbeadfcb-c3b7-4c7a-b7bf-aeea8334951dDesktop
https://github.com/Expensify/App/assets/18525360/e313dbec-f336-4cf0-b790-2a3b19d491d5iOS
https://github.com/Expensify/App/assets/18525360/b4b57df2-48dd-4b84-b97b-0269b32fcfa9Android
https://github.com/Expensify/App/assets/18525360/adfc88ae-db17-4188-8275-33e8d5e73e16Extra - workspace avatar
Uploading workspaceAvatar.mov…