Fix styles for MoneyRequestHeader #18467
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
melvin-bot
bot
requested review from
Beamanator and
rushatgabhane
and removed request for
a team
|
@rushatgabhane @Beamanator 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] |
|
@yuwenmemon should I review this? |
|
No worries @rushatgabhane . I can do that now. it is a follow-up PR. |
|
@rushatgabhane @parasharrajat coming from this post, this is an EC3 issue so whoever can review first should do it. |
|
seems like @parasharrajat is on it now |
|
The corner icon looks a little off although it has the correct padding as other icons. Should reduce the gap for it? |
|
I am suggesting a few style change that looks better to me. Sorry, I could do that on the original PR because I was trying to figure out the testing. Here is the diff of suggested changes. IMO, the flex1 should be applied to AvatarWithDisplayName container and no wrapper should be applied to HeaderWithcloseButton. This will keepHeaderWithcloseButton more flexible for future. diff --git a/src/components/AvatarWithDisplayName.js b/src/components/AvatarWithDisplayName.js
index dfee2ca605..cb56b27b48 100644
--- a/src/components/AvatarWithDisplayName.js
+++ b/src/components/AvatarWithDisplayName.js
@@ -51,59 +51,57 @@ const AvatarWithDisplayName = (props) => {
const ownerPersonalDetails = OptionsListUtils.getPersonalDetailsForLogins([props.report.ownerEmail], props.personalDetails);
const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(ownerPersonalDetails, false);
return (
- <View>
- <View style={[styles.appContentHeaderTitle]}>
- {Boolean(props.report && title) && (
- <View
+ <View style={[styles.appContentHeaderTitle, styles.flex1]}>
+ {Boolean(props.report && title) && (
+ <View
+ style={[
+ styles.flexRow,
+ styles.alignItemsCenter,
+ styles.justifyContentBetween,
+ ]}
+ >
+ {isExpenseReport ? (
+ <SubscriptAvatar
+ mainAvatar={icons[0]}
+ secondaryAvatar={icons[1]}
+ mainTooltip={props.report.ownerEmail}
+ secondaryTooltip={subtitle}
+ size={props.size}
+ />
+ ) : (
+ <Avatar
+ size={props.size}
+ source={icons[0].source}
+ type={icons[0].type}
+ name={icons[0].name}
+ containerStyles={props.size === CONST.AVATAR_SIZE.SMALL ? styles.emptyAvatarSmall : styles.emptyAvatar}
+ />
+ )}
+ <View style={[styles.flex1, styles.flexColumn, styles.ml3]}>
+ <DisplayNames
+ fullTitle={title}
+ displayNamesWithTooltips={displayNamesWithTooltips}
+ tooltipEnabled
+ numberOfLines={1}
+ textStyles={[styles.headerText, styles.pre]}
+ shouldUseFullTitle={isExpenseReport}
+ />
+ {!_.isEmpty(subtitle) && (
+ <Text
style={[
- styles.flexRow,
- styles.alignItemsCenter,
- styles.justifyContentBetween,
+ styles.sidebarLinkText,
+ styles.optionAlternateText,
+ styles.textLabelSupporting,
+ styles.pre,
]}
+ numberOfLines={1}
>
- {isExpenseReport ? (
- <SubscriptAvatar
- mainAvatar={icons[0]}
- secondaryAvatar={icons[1]}
- mainTooltip={props.report.ownerEmail}
- secondaryTooltip={subtitle}
- size={props.size}
- />
- ) : (
- <Avatar
- size={props.size}
- source={icons[0].source}
- type={icons[0].type}
- name={icons[0].name}
- containerStyles={props.size === CONST.AVATAR_SIZE.SMALL ? styles.emptyAvatarSmall : styles.emptyAvatar}
- />
- )}
- <View style={[styles.flex1, styles.flexColumn, styles.ml3]}>
- <DisplayNames
- fullTitle={title}
- displayNamesWithTooltips={displayNamesWithTooltips}
- tooltipEnabled
- numberOfLines={1}
- textStyles={[styles.headerText, styles.pre]}
- shouldUseFullTitle={isExpenseReport}
- />
- {!_.isEmpty(subtitle) && (
- <Text
- style={[
- styles.sidebarLinkText,
- styles.optionAlternateText,
- styles.textLabelSupporting,
- styles.pre,
- ]}
- numberOfLines={1}
- >
- {subtitle}
- </Text>
- )}
- </View>
- </View>
- )}
+ {subtitle}
+ </Text>
+ )}
+ </View>
</View>
+ )}
</View>
);
};
diff --git a/src/components/HeaderWithCloseButton.js b/src/components/HeaderWithCloseButton.js
index 6b012a3ee0..0ec4726558 100755
--- a/src/components/HeaderWithCloseButton.js
+++ b/src/components/HeaderWithCloseButton.js
@@ -165,30 +165,28 @@ class HeaderWithCloseButton extends Component {
styles.overflowHidden,
]}
>
- <View style={[styles.flexRow, this.props.shouldShowAvatarWithDisplay && styles.flex1]}>
- {this.props.shouldShowBackButton && (
- <Tooltip text={this.props.translate('common.back')}>
- <Pressable
- onPress={() => {
- if (this.props.isKeyboardShown) {
- Keyboard.dismiss();
- }
- this.props.onBackButtonPress();
- }}
- style={[styles.touchableButtonImage]}
- >
- <Icon src={Expensicons.BackArrow} />
- </Pressable>
- </Tooltip>
- )}
- {this.props.shouldShowAvatarWithDisplay && (
- <AvatarWithDisplayName
- report={this.props.report}
- policies={this.props.policies}
- personalDetails={this.props.personalDetails}
- />
- )}
- </View>
+ {this.props.shouldShowBackButton && (
+ <Tooltip text={this.props.translate('common.back')}>
+ <Pressable
+ onPress={() => {
+ if (this.props.isKeyboardShown) {
+ Keyboard.dismiss();
+ }
+ this.props.onBackButtonPress();
+ }}
+ style={[styles.touchableButtonImage]}
+ >
+ <Icon src={Expensicons.BackArrow} />
+ </Pressable>
+ </Tooltip>
+ )}
+ {this.props.shouldShowAvatarWithDisplay && (
+ <AvatarWithDisplayName
+ report={this.props.report}
+ policies={this.props.policies}
+ personalDetails={this.props.personalDetails}
+ />
+ )}
{!this.props.shouldShowAvatarWithDisplay && (
<Header
title={this.props.title} |
|
More changes, we should remove the containerStyles from the HeaderWithCloseButton in MoneyRequestHeader. diff --git a/src/components/MoneyRequestHeader.js b/src/components/MoneyRequestHeader.js
index 728ff0a227..df09048c6a 100644
--- a/src/components/MoneyRequestHeader.js
+++ b/src/components/MoneyRequestHeader.js
@@ -73,7 +73,6 @@ const MoneyRequestHeader = (props) => {
report={props.report}
policies={props.policies}
personalDetails={props.personalDetails}
- containerStyles={[styles.pt5, styles.pb3, styles.pr1]}
shouldShowCloseButton={false}
shouldShowBackButton={props.isSmallScreenWidth}
onBackButtonPress={() => Navigation.navigate(ROUTES.HOME)} |
|
Doing the above changes will keep the design as they are bug fixes for a few alignment issues. |
|
Thanks @parasharrajat! Updated! |
|
Tests good @parasharrajat let us know if you wont be able to get to this review |
Reviewer Checklist
Screenshots/VideosWeb
Mobile Web - Chrome
Mobile Web - Safari
Desktopsame as web iOS
Android
|
|
Atually did the testing and we are good to go |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Thanks for taking care of this. I was asleep but I already tested. If suggestion were applied as it is then everything will be good. |
|
The issue should have been created automatically, since the PR points to an E/E internal issue. Not sure why it wasn't created. |
|
Created payment issue here |
|
Its created only when C+ approved the PR and Rajat could not do that after it was merged. |
26 tasks
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.12-0 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.12-0 🚀
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 1.3.12-0 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@luacmartins please review
cc @grgia @parasharrajat
?w=1review reccomendedDetails
After reverting the styles back to what they were originally in this PR: #18465 I went back to the drawing board to see how we could get the header to show properly for when we're using an
AvatarWithDisplayNameFixed Issues
$ https://github.com/Expensify/Expensify/issues/270587
Tests
(Internal, dev)
Apply the following diff to your local code:
Replace the reportID in this line:
... with the reportID of an IOU report you have locally.
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android