-
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
feat: add employee and accounting page to onboarding flow #49161
base: main
Are you sure you want to change the base?
Conversation
@dubielzyk-expensify what are your thoughts on the "Connect your accounting..." text below the headline? I am thinking it should be bigger (in our normal font size, 15px) and maybe a bit more margin below it and above the options? |
The font size is not consistent Screen.Recording.2024-09-16.at.1.14.10.PM.mov |
Agree. In the mocks I've specified it as |
Agreed. All headlines should be the same here. Also the padding here is off on @s77rt 's recording: The text should all line up vertically on the left like so: We use 32px padding on desktop modals and 20px on mobile :) |
This bug is fixed on the latest code. |
Oh, that is the font-size that we use in the onboarding work step and I used this when creating new pages. |
@dubielzyk-expensify How about the spacing between the title and the description in the accounting step? Is it also 20px? |
Update two screenshots for desktop and mobile. Screen.Recording.2024-09-17.at.11.20.45.movScreen.Recording.2024-09-17.at.11.21.47.mov |
Updated. Screen.Recording.2024-09-17.at.12.37.48.movScreen.Recording.2024-09-17.at.12.37.31.mov |
This is a workaround. Please remove it. Also I think the case you mentioned is not worth optimizing for, the case I reported is when we refresh on the accounting page and go back we should have a selection, this should work already without the workaround. The case you mentioned is when we refresh on the employees page, i think it's okay to have no selection. |
@s77rt Updated
Yes, it worked. |
leftElement: onboardingIsMediumOrLargerScreenWidth ? <View style={styles.ml3} /> : null, | ||
rightElement: onboardingIsMediumOrLargerScreenWidth ? <View style={styles.mr3} /> : null, |
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.
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.
As I mentioned, SelectionList
doesn't have a prop to add style to ListItem
. Then if we want to do that, we need to introduce a new prop.
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.
Adding a new prop seems to be the right approach here. Can you please work on that?
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 Updated with a new prop.
Co-authored-by: Abdelhafidh Belalia <[email protected]>
Co-authored-by: Abdelhafidh Belalia <[email protected]>
Co-authored-by: Abdelhafidh Belalia <[email protected]>
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.
Almost there
employees: { | ||
title: 'How many empleados do you have?', | ||
[CONST.ONBOARDING_COMPANY_SIZE.MICRO]: '1-10 empleados', | ||
[CONST.ONBOARDING_COMPANY_SIZE.SMALL]: '11-50 empleados', | ||
[CONST.ONBOARDING_COMPANY_SIZE.MEDIUM_SMALL]: '51-100 empleados', | ||
[CONST.ONBOARDING_COMPANY_SIZE.MEDIUM]: '101-1,000 empleados', | ||
[CONST.ONBOARDING_COMPANY_SIZE.LARGE]: 'More than 1,000 empleados', | ||
}, | ||
accounting: { | ||
title: 'Do you use any accounting software?', | ||
description: 'Connect your accounting software directly to Expensify', | ||
noneOfAbove: 'None of the above', | ||
}, |
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.
@nkdengineer Please ask in Slack for the translations
@nkdengineer Can you please check #49161 (comment) |
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.
Looks good overall. I think we are missing:
- The description in the employees step. Waiting on @dubielzyk-expensify for confirmation feat: add employee and accounting page to onboarding flow #49161 (comment)
- The translations feat: add employee and accounting page to onboarding flow #49161 (comment)
Details
Fixed Issues
$ #48745
PROPOSAL: #48745 (comment)
Tests
Manage my team's expenses
Offline tests
None
QA Steps
Manage my team's expenses
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-09-16.at.15.30.48.mov
Android: mWeb Chrome
Screen.Recording.2024-09-16.at.15.27.18.mov
iOS: Native
Screen.Recording.2024-09-16.at.15.31.36.mov
iOS: mWeb Safari
Screen.Recording.2024-09-16.at.15.26.16.mov
MacOS: Chrome / Safari
Screen.Recording.2024-09-16.at.15.20.11.mov
MacOS: Desktop
Screen.Recording.2024-09-16.at.15.34.54.mov