-
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
feat: New landing page design #3079
feat: New landing page design #3079
Conversation
Looks like the mobile view should have more padding around the inner logo/UI/etc. |
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.
Conflicts and a few comments
src/styles/utilities/spacing.js
Outdated
paddingHorizontal: 24, | ||
}, | ||
|
||
ph7: { |
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.
Is this used? If not, let's remove it and any other unused classes that have been added here. For example, we have ph2 and ph4 above but not ph3 since it isn't used.
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.
Ideally we should keep all the values even if they aren't used.
This won't increase the bundle size as unwanted objects will be tree shaken during compile time
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.
Bundle size isn't the reason we want to avoid adding unused classes, the main reason is to avoid adding unnecessary code that can potentially be used by other contributors. In this case, we don't have a need for ph7, but if it is added in this PR then somebody could use ph7 later on and cause inconsistent styles across the app.
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.
Done
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.
Were these removed? I still see them in the project now, it looks like 7-10 are unused after the latest changes.
fix: Linting issues and removes unwanted props fix: minor styling changes fix: Terms of service component change for native feat: PR review changes and removed unwatned props
72f226c
to
059a4d3
Compare
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.
Added a couple more comments, will let @shawnborton review the latest style
src/styles/utilities/spacing.js
Outdated
paddingHorizontal: 24, | ||
}, | ||
|
||
ph7: { |
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.
Bundle size isn't the reason we want to avoid adding unused classes, the main reason is to avoid adding unnecessary code that can potentially be used by other contributors. In this case, we don't have a need for ph7, but if it is added in this PR then somebody could use ph7 later on and cause inconsistent styles across the app.
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.
This is looking good to me, good work on this! A few things for @shawnborton to check on since I'm not sure if they're real issues:
-
Landscape iOS has a bunch of space at the top. Probably not an issue, but just double-checking w/ design
-
Web with a small window has some tight spacing on the left and bottom, again just double-checking w/ design
Will leave final review to @shawnborton
Sorry for the delay - hoping to review this tomorrow. |
Hi, @shawnborton any update on this? |
You can use this file here and it should be shown at 78px wide: expensify-wordmark.svg.zip |
Just chiming in as well to say I agree with Joe's feedback about this:
|
Also just a heads up that I will be OOO next week, so tagging @michelle-thompson to help out with final design review while I am out. |
Landscape Compatibility and max width Peek.2021-06-06.21-33.mp4 |
Hey @Jag96 I think this is blocked on your review? We have a tight deadline for this one, can you take a look when you have a chance? |
As long as the input field doesn't move around when switching screens, that is what's important here. And I think the landscape view on mobile looks good 👍 |
@shawnborton I have updated the PR with the final changes. |
Thanks for the update! Do you know what's happening with the fine print here? The line height looks strange: And cc @kevinksullivan - should it be "New Expensify" or "new Expensify"? |
@pranshuchittora it also looks like you missed changing out |
Wait somebody again changes the TermsWithLicenses 🤦🏼 |
src/languages/en.js
Outdated
@@ -259,17 +259,26 @@ export default { | |||
}, | |||
signInPage: { | |||
expensifyDotCash: 'Expensify.cash', | |||
expensifyIsOpenSource: 'Expensify.cash is open source', | |||
expensifyIsOpenSource: 'New Expensify is open source', |
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.
@kevinksullivan is this how we want it to read? As stated elsewhere, this just seems clunky/grammatically incorrect to me.
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.
Perhaps "The new Expensify is open source"
?
src/languages/en.js
Outdated
@@ -95,7 +95,7 @@ export default { | |||
hello: 'Hello', | |||
phoneCountryCode: '1', | |||
welcomeText: { | |||
phrase1: 'With Expensify.cash, chat and payments are the same thing.', | |||
phrase1: 'Welcome to new Expensify! Enter your phone or email to continue.', |
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.
This language is confusing. Perhaps 'Welcome to the new Expensify! Enter your phone number or email to continue.'
@kevinksullivan does it not seem weird to you to have it say "Welcome to new Expensify!"
?
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 agree. I think the New Expensify
is best as the latest mockup shows.
@joelbettner I have updated the PR. cc @shawnborton |
@pranshuchittora Thank you - one last change - can you please capitalize the "N" in |
Done @shawnborton |
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.
LGTM!
✋ 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 in version: 1.0.77-6🚀
|
True, but I do think it would be nice if that top margin above the icon used a relative unit. Remember I left that comment here: #3079 (comment) We have this in place on our Expensify.com homepage: Where the top padding is |
🚀 Deployed to production in version: 1.0.79-4🚀
|
Details
New landing page design
Fixed Issues
Closes #2938
Tests
QA Steps
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android