-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Change LHN header to Expensify #18802
Conversation
…thing really) to be rendered inside the header
…be re-used in various places
Nice. And can you make the text in the badge use a bold weight please? |
Great, looks good to me! |
This PR is ready for testing... |
I think it's fine. None of this is customer facing either. |
Asked in #contributor-plus if anyone is available to jump in and test this now. |
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 you can probably undo the changes to this file and the assets/images/expensify-logo--staging.svg
file since we're no longer using them for the LHN stuff.
I'm actually a bit confused how the sign in pages aren't black and still render as the green color... oh nvm it's b/c of this line
App/src/components/ExpensifyWordmark.js
Line 39 in ca85fad
<LogoComponent fill={themeColors.success} /> |
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.
fill={themeColors.success}
prop is actually irrelevant
On Login page, expensify-wordmark.svg
, expensify-logo--dev.svg
, expensify-logo--staging.svg
, expensify-logo--adhoc.svg
are used.
expensify-wordmark.svg
is production logo and doesn't have env badge. But other svgs have env badge
On LHN, only expensify-wordmark.svg
is used. And env badge is fully customized RN view, not image.
As a follow-up, we can remove all these svgs and remain only expensify-wordmark.svg
. So refactor login logo to be same as LHN logo.
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.
Do we have plan on this?
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 don't think that is necessary to both look the same. Also, technically this change is out of the scope of this issue. I am happy to do this but I am busy with other stuff, feel free to spin up a PR if you want to help with this. Thanks.
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.
Tho I'm not crazy we went back to passing this back into header component I don't think we need to block on this and we can always clean it up later as well. Everything else looks good.
Reviewer Checklist
Screenshots/Videos |
going to check on the status of the app deploy to see if we should add cp staging label prior to merge. |
|
Change LHN header to Expensify (cherry picked from commit 05f5b06)
…-18802 🍒 Cherry pick PR #18802 to staging 🍒
🚀 Cherry-picked to staging by https://github.com/bondydaa in version: 1.3.14-14 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.14-14 🚀
|
🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
Details
Continuing the work #18449.
Fixed Issues
$ #18382
Tests
LHN(Chat list)
Signin Page
You can test the various badges locally by editing:
App/src/components/ExpensifyWordmark.js
Line 41 in a29631b
and use the constants from above: CONST.ENVIRONMENT.DEV, CONST.ENVIRONMENT.STAGING, CONST.ENVIRONMENT.PRODUCTION
Offline tests
QA Steps
Same as tests.
Badge editing does not apply. Instead, you should be seeing
STG
badge on staging and no badge on production.Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android