Change LHN header to Expensify #18449
Conversation
melvin-bot
bot
requested review from
cristipaval and
parasharrajat
and removed request for
a team
|
@parasharrajat @cristipaval 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] |
src/languages/en.js
Outdated
| @@ -295,7 +295,7 @@ export default { | |||
| newChat: 'New chat', | |||
| newGroup: 'New group', | |||
| newRoom: 'New room', | |||
| headerChat: 'Chats', | |||
| headerChat: 'Expensify', | |||
There was a problem hiding this comment.
let's call it headerTitle
Update: need to remove this completely.
There was a problem hiding this comment.
We have to use the wordmark SVG https://github.com/Expensify/App/blob/3585329ec6fe8a8bed127fc44b06ff698380a440/assets/images/expensify-wordmark.svg instead of Text as per #18382 (comment)
…thing really) to be rendered inside the header
|
okay if you look at these 2 commits that's how I first tried to get this to work as per the thread here: https://expensify.slack.com/archives/C01GTK53T8Q/p1683315638909159 But that led to this linter error: So that's why I changed it and instead have a new component that is able to just render it's children. We could also refactor |
|
also I still can't get iOS or android to build locally so I can't test there 😢 I'll see if I can get that figured out next week but in the meantime if @parasharrajat could test and get those screen shots that'd be 💯 ❤️ |
|
I still think we can easily adjust the Header component to have an image. Here is the diff of my attempt. This won't require changing the Header usage everywhere. diff --git a/src/components/Header.js b/src/components/Header.js
index 01752e8e07..8e7747bdb1 100644
--- a/src/components/Header.js
+++ b/src/components/Header.js
@@ -8,7 +8,7 @@ import EnvironmentBadge from './EnvironmentBadge';
const propTypes = {
/** Title of the Header */
- title: PropTypes.string.isRequired,
+ title: PropTypes.oneOfType([PropTypes.string, PropTypes.element]).isRequired,
/** Subtitle of the header */
subtitle: PropTypes.oneOfType([PropTypes.string, PropTypes.element]),
@@ -29,9 +29,15 @@ const defaultProps = {
const Header = props => (
<View style={[styles.flex1, styles.flexRow]}>
<View style={styles.mw100}>
- <Text numberOfLines={2} style={[styles.headerText, styles.textLarge, ...props.textStyles]}>
- {props.title}
- </Text>
+ {_.isString(props.title)
+ ? (
+ Boolean(props.title) && (
+ <Text numberOfLines={2} style={[styles.headerText, styles.textLarge, ...props.textStyles]}>
+ {props.title}
+ </Text>
+ )
+ )
+ : props.title}
{/* If there's no subtitle then display a fragment to avoid an empty space which moves the main title */}
{_.isString(props.subtitle)
? Boolean(props.subtitle) && <Text style={[styles.mutedTextLabel, styles.pre]} numberOfLines={1}>{props.subtitle}</Text>
diff --git a/src/components/ImageHeader.js b/src/components/ImageHeader.js
deleted file mode 100644
index 5c82006e39..0000000000
--- a/src/components/ImageHeader.js
+++ /dev/null
@@ -1,40 +0,0 @@
-import React from 'react';
-import {View} from 'react-native';
-import PropTypes from 'prop-types';
-import _ from 'underscore';
-import styles from '../styles/styles';
-import EnvironmentBadge from './EnvironmentBadge';
-
-const propTypes = {
- children: PropTypes.node.isRequired,
-
- /** Subtitle of the header */
- subtitle: PropTypes.oneOfType([PropTypes.string, PropTypes.element]),
-
- /** Should we show the environment badge (dev/stg)? */
- shouldShowEnvironmentBadge: PropTypes.bool,
-};
-
-const defaultProps = {
- shouldShowEnvironmentBadge: false,
- subtitle: '',
-};
-const ImageHeader = props => (
- <View style={[styles.flex1, styles.flexRow]}>
- <View style={styles.mw100}>
- {props.children}
- {/* If there's no subtitle then display a fragment to avoid an empty space which moves the main title */}
- {_.isString(props.subtitle)
- ? Boolean(props.subtitle) && <Text style={[styles.mutedTextLabel, styles.pre]} numberOfLines={1}>{props.subtitle}</Text>
- : props.subtitle}
- </View>
- {props.shouldShowEnvironmentBadge && (
- <EnvironmentBadge />
- )}
- </View>
-);
-
-ImageHeader.displayName = 'ImageHeader';
-ImageHeader.propTypes = propTypes;
-ImageHeader.defaultProps = defaultProps;
-export default ImageHeader;
diff --git a/src/pages/home/sidebar/SidebarLinks.js b/src/pages/home/sidebar/SidebarLinks.js
index 57f2df7b2c..17890ca5ff 100644
--- a/src/pages/home/sidebar/SidebarLinks.js
+++ b/src/pages/home/sidebar/SidebarLinks.js
@@ -14,7 +14,7 @@ import compose from '../../../libs/compose';
import Navigation from '../../../libs/Navigation/Navigation';
import ROUTES from '../../../ROUTES';
import Icon from '../../../components/Icon';
-import ImageHeader from '../../../components/ImageHeader';
+import Header from '../../../components/Header';
import * as Expensicons from '../../../components/Icon/Expensicons';
import AvatarWithIndicator from '../../../components/AvatarWithIndicator';
import Tooltip from '../../../components/Tooltip';
@@ -155,13 +155,12 @@ class SidebarLinks extends React.Component {
]}
nativeID="drag-area"
>
- <ImageHeader
- accessibilityLabel={"Expensify"}
+ <Header
+ title={<LogoWordmark width={108} fill={defaultTheme.textLight} />}
+ accessibilityLabel="Expensify"
accessibilityRole="text"
shouldShowEnvironmentBadge
- >
- <LogoWordmark width={108} fill={defaultTheme.textLight} />
- </ImageHeader>
+ />
<Tooltip text={this.props.translate('common.search')}>
<TouchableOpacity
accessibilityLabel={this.props.translate('sidebarScreen.buttonSearch')} |
|
How about we use the wordmark logos we have that already have the staging/dev/ad hoc labels baked in? Like this from the sign in page: |
|
@bondydaa Just letting you know that I will be offline after 5 hours. So let's finish this before that. |
|
Ah didn't know we had those other word mark svgs can update. This component is becoming pretty specialized so I think we should leave it as it's own component then. The header component then can have all of the extra logic for the environment variables removed. I think it'll be unlikely we need to reuse this new component then but that's fine. |
|
oh we have a wordmark component already 😅 https://github.com/Expensify/App/blob/0bc7607bf3/src/components/ExpensifyWordmark.js#L32 pretty sure I can just use that then |
…be re-used in various places
Is there any reason why we need to touch those images? I think you should just leave those images as they are, and then for the LHN let's just replace the word "Chats" with a wordmark SVG (which already exists in the repo) and then use CSS to style up the existing environment badges to make them appear to be smaller. |
|
@bondydaa Let me know if you need help with anything on this. I am thinking that this issue has EC3 priority so looking forward to close this fast. |
|
It seems silly to me have 2 different ways of displaying the Wordmark and environment badge: One for the sign in pages (which is a SVG with both wordmark and environment badge) and then for LHN of SVG + Environment badge component. The way I have it avoids needing to throw a lot of custom logic into the Header component just for the LHN or avoid having 2 components that are basically the same with slightly different usages. Are the SVG images I used the wrong font-family for the wordmark? |
|
Ok, I took another look at the code and now I understand how it works. Basically, we are removing the hardcoded color from the svgs and then passing it from the code. This way we can reuse the same SVG anywhere we want and change the color for it. That's why the SVG got changed. |
src/components/ExpensifyWordmark.js
Outdated
| import * as StyleUtils from '../styles/StyleUtils'; | ||
| import variables from '../styles/variables'; | ||
|
|
||
| const propTypes = { | ||
| ...environmentPropTypes, | ||
| ...windowDimensionsPropTypes, | ||
|
|
||
| /** The styles to apply for the View wrapping the svg */ | ||
| containerStyles: PropTypes.arrayOf(PropTypes.object), |
There was a problem hiding this comment.
Please use StylePropTypes for this.
|
Alrighty fixed the proptypes, only thing is the production version of the wordmark being slightly indented... maybe it has to do with the width of the element & flexbox since when the badges are present it's aligned properly? |
|
I think the image size is a little bit big. cc @shawnborton |
|
BUG: Web: Search bar icon is not aligned right. |
| @@ -49,7 +52,9 @@ const SignInPageContent = props => ( | |||
| <View style={[styles.flexGrow2, styles.mb8]}> | |||
| <SignInPageForm style={[styles.alignSelfStretch]}> | |||
| <View style={[props.isSmallScreenWidth ? styles.mb8 : styles.mb15, props.isSmallScreenWidth ? styles.alignItemsCenter : styles.alignSelfStart]}> | |||
| <ExpensifyWordmark /> | |||
| <ExpensifyWordmark | |||
| containerStyles={[(props.isSmallScreenWidth && (props.environment === CONST.ENVIRONMENT.DEV || props.environment === CONST.ENVIRONMENT.STAGING)) ? styles.ml3 : {}]} | |||
There was a problem hiding this comment.
Has a lint issue. Please wrap this in next lines.
| shouldShowEnvironmentBadge | ||
| textStyles={[styles.textHeadline]} | ||
| /> | ||
| <View> |
There was a problem hiding this comment.
Add flexRow and flex1 to fix the search icon.
|
Following #18449 (comment), I applied 130x30 to test and it looks like this
|
|
Nice, that looks better. The idea was that the wordmark portion would be 108px wide, and then we have to account for the extra width for the versions that have badges. |
Is this still happening? And can you confirm that the existing sign up page looks exactly like it does on production too? |
|
Hmm just had a thought... @parasharrajat do you want to take over this PR and get it across the finish line? There no reason to have me a be a blocker here while I'm focused on Manual Request stuff. if so feel free to assign yourself and move me to a reviewer. |
|
That works. but I can't push to your branch. I will have to reopen a new PR. |
|
oh hmm... wonder if we could CP the changes over or something if you give me write access to your repo/fork/branch? whatever is easiest just let me know |
|
or actually I can also just post the diff and you can |
|
New PR #18802 |
|
I have made adjustments to the wordmark dimensions on the new PR and it is ready for review. We can close this PR. |
|
@bondydaa looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
hmm weird what? I merged #18802 not this |
|
This happened because I used your commits and didn't replace them. So we should have closed this first before merging other. |
|
oh interesting! whoops yes I def should have closed this first 🤦 |
Details
This updates the "header" to show
Expensifyper the design mocks on the linked issue.Fixed Issues
$ #18382
Tests
LHN
Expensifyand notChatsand has the proper environment badgeSignin
You can test the various badges locally by editing:
App/src/components/ExpensifyWordmark.js
Line 41 in a29631b
CONST.ENVIRONMENT.DEV,CONST.ENVIRONMENT.STAGING,CONST.ENVIRONMENT.PRODUCTIONOffline tests
Nothing specific really but same it should say
ExpensifynotChatat top of LHNQA Steps
LHN
Expensifyand notChatsand has the proper environment badgeSignin
Wash, rinse, repeat for Staging and Production
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
Dev
Signin
LHN
Staging
Signin
LHN
Production
Signin
LHN
Mobile Web - Chrome
Dev
Signin
LHN
Staging
Signin
LHN
Production
Signin
LHN
Mobile Web - Safari
Desktop
Dev
Signin
LHN
Staging
Signin
LHN
Production
Signin
LHN
iOS
Android