-
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
Move checkmark icon to right of settled amount in request money report #19524
Conversation
@Julesssss @aimane-chnaif 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] |
@aimane-chnaif PR is ready for review. |
src/components/MenuItem.js
Outdated
@@ -36,6 +37,8 @@ const defaultProps = { | |||
wrapperStyle: [], | |||
style: styles.popoverMenuItem, | |||
titleStyle: {}, | |||
shouldShowTitleRightIcon: false, | |||
titleRightIcon: () => {}, |
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 titleIcon
is better
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.
Updated.
/** Boolean whether to display the title right icon */ | ||
shouldShowTitleIcon: PropTypes.bool, | ||
|
||
/** Icon to display at right side of title */ | ||
titleIcon: PropTypes.func, | ||
|
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.
That's better, thanks!
Seems ready for review @aimane-chnaif |
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemchrome.movMobile Web - Safarimsafari.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
@aimane-chnaif Pushed commit to handle very long title in MenuItem. This will solve extreme condition. |
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.
@Julesssss LGTM 🎉
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.19-2 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.19-7 🚀
|
@@ -66,6 +69,7 @@ const MenuItem = (props) => { | |||
const descriptionVerticalMargin = props.shouldShowDescriptionOnTop ? styles.mb1 : styles.mt1; | |||
const titleTextStyle = StyleUtils.combineStyles( | |||
[ | |||
styles.flexShrink1, |
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.
Because we're also using white-space: pre
in this styling the calculated width of Text could be smaller and hence might show ellipses. This caused a regression in the date picker component.
{convertToLTR(props.title)} | ||
</Text> | ||
)} | ||
<View style={[styles.flexRow, styles.alignItemsCenter]}> |
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 this view around the Text
with these styles caused a problem with text truncation on Android, which we dealt with in #23769.
@aimane-chnaif @Julesssss PR is ready for review.
Details
When Request money report open which is Settled, previously it was showing Green checkmark icon at right side of the "To" header. So In this pr we removed checkmark icon from right side of "To" header, and placed it at right side of the settled amount. We also added "Settled" word within description line as shown in screenshot.
Note: In this PR we did changes related settled money request report. Although, we provided screenshot for unsettled report to show that there is not any side effect.
Fixed Issues
$ #19332
PROPOSAL: #19332 (comment)
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 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
Chrome
Safari
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android