-
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
Export main page - initial #39191
Export main page - initial #39191
Conversation
# Conflicts: # src/languages/es.ts # src/libs/Navigation/linkingConfig/FULL_SCREEN_TO_RHP_MAPPING.ts # src/libs/Navigation/linkingConfig/config.ts # src/libs/Navigation/types.ts # src/pages/workspace/accounting/WorkspaceAccountingPage.tsx # src/types/onyx/Policy.ts
@shawnborton @Expensify/design Hey! Can i ask about first look at Export screens - probably not so many UI changes - but will be nice to get initial feedback. You will see that nothing is selected in the list - for now i'm working on some fake onyx integration - to make screens interactive - but should be ready earlier next week - after that we can run builds to see live. But for now just screenshots for initial review. Thanks! |
For this particular screen, the hint text seems to be too far below the push row (maybe just by 4px or so?) and the line height of the hint text seems too tall. It should be the same line height we use for other label text, which I believe is 16px? |
please mark this PR ready for review when the PR is ready |
src/pages/workspace/accounting/qbo/export/QuickbooksExportConfigurationPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/qbo/export/QuickbooksExportConfigurationPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/qbo/export/QuickbooksOutOfPocketExpenseConfigurationPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/qbo/export/QuickbooksExportConfigurationPage.tsx
Outdated
Show resolved
Hide resolved
src/pages/workspace/accounting/qbo/export/QuickbooksExportConfigurationPage.tsx
Outdated
Show resolved
Hide resolved
onPress={() => Navigation.navigate(ROUTES.POLICY_ACCOUNTING_QUICKBOOKS_ONLINE_EXPORT_OUT_OF_POCKET_EXPENSES_ACCOUNT_SELECT.getRoute(policyID))} | ||
brickRoadIndicator={errors?.exportAccount ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : undefined} | ||
shouldShowRightIcon | ||
error={errors?.exportAccount ? translate('common.genericErrorMessage') : undefined} |
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.
Same question ^
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.
@s77rt i think for now it will be default one - nothing was specified in high level doc, and after fixing errorText we will move to api errors directly
Reviewer Checklist
Screenshots/Videos |
@s77t, where are we at with this PR? are we close to merging the PR? @narefyev91, there is one merge conflicts, can you solve it when you come back online? |
@hayata-suenaga Yes, this is pretty much ready. The above comments should be easy to address. |
# Conflicts: # src/libs/PolicyUtils.ts
Seems that this one is still unresolved #39191 (comment) |
@s77rt Not really sure why we should combine both export entity or export account as a indicator for this field? I'm not seeing connected logic between export entity and export account. |
Both are in the inner pages of that menu item.
If either menu item 1 or 2 have a BRB then menu item A should have it too. Otherwise you wouldn't know that we have errors |
got it. Updated |
Thanks! Code looks good to me. I'm currently outside, once I get back I will give this the final test. Feel free to merge though as I don't see any blockers. |
@hayata-suenaga @shawnborton can someone from you create a follow up github issue - for Error text component based on this discussion - #39191 (comment) - i can probably with that as well |
I noticed that if we have an error in the preferred exporter we can't clear it if there is no other option to choose. This not a blocker. This is due to missing the failure logic in Screen.Recording.2024-04-22.at.2.36.02.PM.mov |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Created one 😄 @narefyev91, could you comment something on the issue so that I can assign you there? 😄 |
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.
🟢 thank you so much everyone for pushing forward this PR!
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.64-6 🚀
|
Details
Export main pages for QBO
Fixed Issues
$ #37780
PROPOSAL:
Tests
Offline tests
QA Steps
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
android.mov
Android: mWeb Chrome
android-web.mov
iOS: Native
Screen.Recording.2024-04-19.at.20.30.26.mov
iOS: mWeb Safari
Screen.Recording.2024-04-17.at.16.59.37.mov
MacOS: Chrome / Safari
Screen.Recording.2024-04-17.at.16.49.03.mov
MacOS: Desktop
Screen.Recording.2024-04-17.at.17.07.43.mov