-
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 |
{ | ||
firstName: '', | ||
lastName: '', | ||
}, |
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.
This should be optional.
(To recheck: the BE seems to complain about the empty names)
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.
Should we also update the default value of this data in compleOnboarding
function with the default value as empty string?
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.
Make it optional in completeOnboarding
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.
AFAICT it is optional. Lmk what you are seeing. Yes I would update this to be optional.
I am also pretty confused about why it's being passed as an object if we can fix that while we are here. Maybe someone thought it better to parameterize the arguments? But the next person disagreed 😄
Looks inconsistent to me. Let's make it:
function completeOnboarding(
engagementChoice: OnboardingPurposeType,
data: ValueOf<typeof CONST.ONBOARDING_MESSAGES>,
firstName?: string;
lastName?: string;
adminsChatReportID?: string,
onboardingPolicyID?: string,
paymentSelected?: string,
...
) {
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.
{
"jsonCode": 402,
"message": "402 Missing firstName",
"onyxData": [],
"requestID": "8c542209db944e20-ORN"
}
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.
Sigh... lol 😄
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.
@marcaaron Should we add default value for firstName
and lastName
in completeOnboarding
because in the type CompleteGuidedSetupParams
, firstName
and lastName
is string.
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.
Yes let's do it. We will need it in some cases, but not for the "manage team expense" choice so it is appropriate to make optional.
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.
I 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.
+Conflicts
Let's init the employees value from onyx Screen.Recording.2024-09-18.at.9.23.10.PM.mov |
{ | ||
firstName: '', | ||
lastName: '', | ||
}, |
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.
AFAICT it is optional. Lmk what you are seeing. Yes I would update this to be optional.
I am also pretty confused about why it's being passed as an object if we can fix that while we are here. Maybe someone thought it better to parameterize the arguments? But the next person disagreed 😄
Looks inconsistent to me. Let's make it:
function completeOnboarding(
engagementChoice: OnboardingPurposeType,
data: ValueOf<typeof CONST.ONBOARDING_MESSAGES>,
firstName?: string;
lastName?: string;
adminsChatReportID?: string,
onboardingPolicyID?: string,
paymentSelected?: string,
...
) {
We have a problem in this case: the first time the component is rendered (reloading this page), I added a solution that is to add an What do you think? Let's me know if you have another idea. |
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.
Co-authored-by: Abdelhafidh Belalia <16493223+s77rt@users.noreply.github.com>
Co-authored-by: Abdelhafidh Belalia <16493223+s77rt@users.noreply.github.com>
Co-authored-by: Abdelhafidh Belalia <16493223+s77rt@users.noreply.github.com>
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