-
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
feat: Provide education/confirmation before creating workspaces in New Workspace flows #53845
base: main
Are you sure you want to change the base?
feat: Provide education/confirmation before creating workspaces in New Workspace flows #53845
Conversation
…space flows. Signed-off-by: krishna2323 <[email protected]>
…space flows. Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
@allgandalf, the regression issues are fixed but I'm facing a new error, can you please take a look? Monosnap.screencast.2024-12-11.13-55-01.mp4 |
Can you merge main and retry ? is this constantly reproducible ? |
I'm still able to reproduce this issue even after merging main. I'm investigating the RCA of the issue. @allgandalf please let me know if you know anything about this, this is probably related to how we are updating the onyx value. It only occurs when creating a second workspace with an avatar. I'm unsure if this is a backend or frontend bug. |
Can you put out reproduction steps please |
Reproduction Steps:
|
I can also reproduce the issue. You can see it here: |
src/libs/actions/Policy/Policy.ts
Outdated
@@ -1891,6 +1910,8 @@ function buildPolicyData(policyOwnerEmail = '', makeMeAdmin = false, policyName | |||
customUnitID, | |||
customUnitRateID, | |||
engagementChoice, | |||
currency: outputCurrency, | |||
file, |
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.
file, | |
file: {...file}, |
Please soft clone it to remove non-indexable errors.
src/libs/actions/Policy/Policy.ts
Outdated
@@ -2007,6 +2038,8 @@ function createDraftWorkspace(policyOwnerEmail = '', makeMeAdmin = false, policy | |||
expenseCreatedReportActionID, | |||
customUnitID, | |||
customUnitRateID, | |||
currency: outputCurrency, | |||
file, |
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.
file, | |
file: {...file}, |
Please soft clone it to remove non-indexable errors.
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.
Thanks @shubham1206agra 🙇🏻, checking...
@Krishna2323 Can you ready this PR with fixes? I need this PR merged today. So, I can work on another feature which requires this screen. |
@@ -3723,7 +3723,7 @@ const translations = { | |||
}, | |||
emptyWorkspace: { | |||
title: 'Create a workspace', | |||
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more -- all at the speed of chat.', | |||
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more — all at the speed of chat.', |
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.
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more — all at the speed of chat.', | |
subtitle: 'Create a workspace to track receipts, reimburse expenses, manage travel, send invoices, and more — all at the speed of chat.', |
Update the copy as per #53753 (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.
I think this update can be made in this PR.
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.
Alright, thanks
Signed-off-by: krishna2323 <[email protected]>
@allgandalf, you can review the code, I will add the screenshot after few hours. |
all good, please take a look at the failing test as well |
Not sure why the BE is not returning the correct avatar. Will check again. |
Signed-off-by: krishna2323 <[email protected]>
🚧 @shawnborton has triggered a test build. You can view the workflow run here. |
@Krishna2323 please fix eslint |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
This comment has been minimized.
This comment has been minimized.
Signed-off-by: krishna2323 <[email protected]>
@allgandalf lint issues fixed. |
@Krishna2323 can you please merge main? the iOS build is failing on current branch, we fixed iOS build with latest commits on main |
I will complete other platform testing in the meantime |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
@Krishna2323 can you please update native platform videos |
Signed-off-by: krishna2323 <[email protected]>
native platform videos uploaded, was facing build issues last time. |
Asked for a new ad-hoc build |
I can kick it off 🚀 |
🚧 @shawnborton has triggered a test build. You can view the workflow run here. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Screen.Recording.2024-12-24.at.10.48.13.PM.mov |
( we discussed this solution on the other PR please implement that) Screen.Recording.2024-12-24.at.10.52.01.PM.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.
I will do a review again tomorrow morning, I guess there isn't much left to change, looks good overall
@@ -3723,7 +3723,7 @@ const translations = { | |||
}, | |||
emptyWorkspace: { | |||
title: 'Create a workspace', | |||
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more -- all at the speed of chat.', | |||
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more — all at the speed of chat.', |
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.
Alright, thanks
@allgandalf, this isn't related to our PR, it can also be reproduced using this adhoc build. |
Signed-off-by: krishna2323 <[email protected]>
Signed-off-by: krishna2323 <[email protected]>
const createFile = (file: File): FileObject => { | ||
if (getPlatform() === 'web') { | ||
return new File([file], file.name, { | ||
type: file.type, | ||
lastModified: file.lastModified, | ||
}); | ||
} | ||
return { | ||
uri: file.uri, | ||
name: file.name, | ||
type: file.type, | ||
}; | ||
}; |
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.
@Krishna2323 Can you tell us why did you create this method 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.
The File API is part of the web platform and is not supported on native iOS and Android environments. This function ensures compatibility by creating a platform-specific file object, handling both web and native platforms appropriately.
Signed-off-by: krishna2323 <[email protected]>
I can't reproduce this bug. Monosnap.screencast.2024-12-27.04-11-44.mp4 |
Explanation of Change
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Workspace - Unable to save new workspace name #53765
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Create workspace - Selected currecy does not appear selected with a green checkmark #53766
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Create workspace - Edit avatar modal is blocking the avatar #53768
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Create workspace - Error message does not have left and right padding #53771
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] FAB - Page crashes after tracking an expense from QAB #53793
Fixed Issues
$ #51504
PROPOSAL: #51504 (comment)
Tests
Create workspace
Create workspace
> Enter any custom nameNew workspace
> Verify RHP is opened with avatar selector, name input and currency selector.New Workspace
buttonOffline tests
Create workspace
Create workspace
> Enter any custom nameNew workspace
> Verify RHP is opened with avatar selector, name input and currency selector.New Workspace
buttonQA Steps
Create workspace
Create workspace
> Enter any custom nameNew workspace
> Verify RHP is opened with avatar selector, name input and currency selector.New Workspace
buttonPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4