-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[QBD] Handle the initial connection for QBD #50216
[QBD] Handle the initial connection for QBD #50216
Conversation
@Expensify/design hey team, I'm implementing this screen and I realize that this screen is not included in the Figma link Can someone help to add it to the Figma link so I can export the computer icon and copy text colors. Thank you. |
Hi @ZhenjaHorbach @lakchote This PR is still waiting for some minor UI changes here and translation confirmation here. But given this project is urgent, I would like to have early feedback on this PR, please help me review it when you have time. Thank you. |
Changes look good ! |
src/languages/en.ts
Outdated
}, | ||
setupPage: { | ||
title: 'Open this link to connect', | ||
body: 'To complete setup, open the following link on the computer where QuickBooks Desktop is running', |
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.
#50216 (comment)
As I can see we have dot here
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.
Oh yeah, the mockup is too small. Let's wait response here then I will update 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.
Agree !
Oh, nice catch. I think we should follow Figma design. I will update it soon. |
@hoangzinh Added that screen here in Figma Here are the illustrations:
I don't know why the screen looked like that in Figma, but I've updated it and it should look like this. Basically just following the normal padding conventions we have for screens like this. No special custom max-width. |
Great! Thank you @dannymcclain |
@hoangzinh this is going in the right direction. Once the PR will be ready, I'll trigger an ad hoc build so Design team can review it. |
Co-authored-by: Lucien Akchoté <lucien@expensify.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.
Left a comment other than that looks good!
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@Expensify/design feel free to test if everything looks good! |
Happy to test. I can't find |
@dubielzyk-expensify you can log in with an The beta's code hasn't been deployed to prod yet, so it might explain why you didn't find the beta. |
This project has a tight deadline. So it would be great if we could merge this PR soon. It would also unblock some other issues. Let us know if you need any other help to verify this PR @dubielzyk-expensify |
Sure @dubielzyk-expensify these are quick screenshots after updated |
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.
Great stuff! Looks good to me from a visual end. I'll let the code people take the rest 😄
Thank you so much for your help @dubielzyk-expensify |
all yours @lakchote @ZhenjaHorbach |
Looks good ! |
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
✋ 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/lakchote in version: 9.0.47-1 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.47-4 🚀
|
Details
Fixed Issues
$ #49697
PROPOSAL:
Tests
Prerequisites: Turn on Beta
quickbooksDesktopOnNewDot
on your accountIn Web/Desktop
quickbooksDesktopOnNewDot
In mWeb/Native apps
quickbooksDesktopOnNewDot
Offline tests
Unable to test in offline
QA Steps
Same as above
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.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
Android: Native
Screen.Recording.2024-10-05.at.22.26.52.mov
Android: mWeb Chrome
Screen.Recording.2024-10-05.at.22.01.32.android.chrome.mov
iOS: Native
Screen.Recording.2024-10-05.at.21.57.34.ios.mov
iOS: mWeb Safari
Screen.Recording.2024-10-05.at.21.58.30.ios.safari.mov
MacOS: Chrome / Safari
Screen.Recording.2024-10-05.at.20.58.02.web.mov
MacOS: Desktop
Screen.Recording.2024-10-05.at.21.02.33.desktop.mov