-
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
Design clean-up and consistency across expense reports and requests #26603
Conversation
…/26138-design-clean-up-report-request
…/26138-design-clean-up-report-request
This reverts commit 5e66460.
…/26138-design-clean-up-report-request
@shawnborton @rushatgabhane @luacmartins One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
So this is an admin on its own workspace right? I think this will be because the smartscans are not valid and we dont handle the errors well. Its not critical for the conference |
Pushed in some small clean up I missed before |
Reviewer Checklist
Screenshots/VideosWebMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
Bug: [mobile] hover flickering on long-press (happening on main) Screen.Recording.2023-09-04.at.11.53.15.AM.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.
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.
Tests well for me 👍
✋ 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/Julesssss in version: 1.3.63-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.63-2 🚀
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.64-0 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.3.64-2 🚀
|
// Show the merchant for IOUs and expenses only if they are custom or not related to scanning smartscan | ||
const shouldShowMerchant = | ||
!_.isEmpty(requestMerchant) && !props.isBillSplit && requestMerchant !== CONST.TRANSACTION.PARTIAL_TRANSACTION_MERCHANT && requestMerchant !== CONST.TRANSACTION.DEFAULT_MERCHANT; | ||
const shouldShowDescription = !_.isEmpty(description) && !shouldShowMerchant; |
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.
We should have checked if the receipt was not being scanned here. Not doing so caused #31104.
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.
Okay, thank you for letting me know. AFAIK, the bug already fixed.
Details
The app has several inaccuracies for previewing a description and merchant of expense reports and expense requests. Also, we are fixing the whisper effect on messages and removing right carets form all report and request previews.
NOTE: the initial proposal has more items to implement, but not all of them were decided to fix. Please read the "Tests" block below to check all fixed scenarios.
Fixed Issues
$ #26138
PROPOSAL: #26138 (comment)
Tests
1. We're showing right carets for all report and request previews, when instead right carets should be removed from all request and report previews.
2. Similarly,
IOU
reports are showing the merchant when they should instead be showing the description (becauseIOUs
always have the same merchant, which is **Request**).3. When a merchant name exists for an expense / request preview, we only show the merchant.
4. Normal "Cash" requests are missing the
X requests
logic.5. Receipt whisper cards are no visible when they are inside "Only visible for you" message.
6. (none) is showing as a merchant for receipts being scanned
(none)
as a merchant(none)
7. Rounded bottom corners of the images in the report preview
8. On Report Preview for requests made to Saastr or SBE workspaces, there is no user email in the workspace name
SBE owes $xx.xx
orSaaStr owes $xx.xx
and there is no email prepending the SBEOffline tests
Same as "Tests"
QA Steps
Same as "Tests"
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
1. We're showing right carets for all report and request previews, when instead right carets should be removed from all request and report previews.
2. Similarly,
IOU
reports are showing the merchant when they should instead be showing the description (becauseIOUs
always have the same merchant, which is **Request**).3. When a merchant name exists for an expense / request preview, we only show the merchant.
4. Normal "Cash" requests are missing the
X requests
logic.5. Cards are no visible when they are inside "Only visible for you" message.
6. (none) is not showing for the smartscanning receipts
7. Rounded bottom corners of the images in the report preview
8. On Report Preview for requests made to Saastr or SBE workspaces, there is no user email in the workspace name
Mobile Web - Chrome
1. We're showing right carets for all report and request previews, when instead right carets should be removed from all request and report previews.
2. Similarly,
IOU
reports are showing the merchant when they should instead be showing the description (becauseIOUs
always have the same merchant, which is **Request**).3. When a merchant name exists for an expense / request preview, we only show the merchant.
4. Normal "Cash" requests are missing the
X requests
logic.5. Cards are no visible when they are inside "Only visible for you" message.
Mobile Web - Safari
1. We're showing right carets for all report and request previews, when instead right carets should be removed from all request and report previews.
2. Similarly,
IOU
reports are showing the merchant when they should instead be showing the description (becauseIOUs
always have the same merchant, which is **Request**).3. When a merchant name exists for an expense / request preview, we only show the merchant.
4. Normal "Cash" requests are missing the
X requests
logic.5. Cards are no visible when they are inside "Only visible for you" message.
Desktop
1. We're showing right carets for all report and request previews, when instead right carets should be removed from all request and report previews.
2. Similarly,
IOU
reports are showing the merchant when they should instead be showing the description (becauseIOUs
always have the same merchant, which is **Request**).3. When a merchant name exists for an expense / request preview, we only show the merchant.
4. Normal "Cash" requests are missing the
X requests
logic.5. Cards are no visible when they are inside "Only visible for you" message.
iOS
1. We're showing right carets for all report and request previews, when instead right carets should be removed from all request and report previews.
2. Similarly,
IOU
reports are showing the merchant when they should instead be showing the description (becauseIOUs
always have the same merchant, which is **Request**).3. When a merchant name exists for an expense / request preview, we only show the merchant.
4. Normal "Cash" requests are missing the
X requests
logic.5. Cards are no visible when they are inside "Only visible for you" message.
Android
1. We're showing right carets for all report and request previews, when instead right carets should be removed from all request and report previews.
2. Similarly,
IOU
reports are showing the merchant when they should instead be showing the description (becauseIOUs
always have the same merchant, which is **Request**).3. When a merchant name exists for an expense / request preview, we only show the merchant.
4. Normal "Cash" requests are missing the
X requests
logic.5. Cards are no visible when they are inside "Only visible for you" message.