-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add lounge access page for users who qualify for lounge membership #20881
Conversation
@narefyev91 Please 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] |
Placing a HOLD as we're still waiting on copy from @mallenexpensify |
Removed hold, copy and notes added here |
Still waiting on Spanish translations. |
Allright we got it. This is ready to go! |
}; | ||
|
||
function LoungeAccessPage(props) { | ||
const menuItems = [ |
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 we can place menuItems outside of function
title={props.translate('loungeAccessPage.loungeAccess')} | ||
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)} | ||
/> | ||
<ScrollView contentContainerStyle={[safeAreaPaddingBottomStyle]}> |
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.
you do not need [] here - you are using only one style
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)} | ||
/> | ||
<ScrollView contentContainerStyle={[safeAreaPaddingBottomStyle]}> | ||
<View style={[illustrationStyle]}> |
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.
same here you can avoid using []
<View style={[illustrationStyle]}> | ||
<Illustrations.Lounge /> | ||
</View> | ||
<View style={[styles.pageWrapperNotCentered]}> |
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.
same here you can avoid using []
> | ||
{props.translate('loungeAccessPage.headline')} | ||
</Text> | ||
<Text style={[styles.baseFontStyle]}>{props.translate('loungeAccessPage.description')}</Text> |
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.
same here you can avoid using []
Thanks @narefyev91 - updated! |
Adding myself as a reviewer, though I likely can't get to it until tomorrow morning. |
@yuwenmemon Hey - can you please check why does your PR showing soooo many line changes |
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.
@narefyev91 - I think this file is why there's so many line changes 😅
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.
Yep - it's cause of the SVG files
Reviewer Checklist
Screenshots/Videos |
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!
🎀 👀 🎀 C+ reviewed
🎯 @narefyev91, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #21306. |
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.
Some minor comments! @Expensify/design do you want to review too?
src/components/Icon/Illustrations.js
Outdated
@@ -28,6 +31,7 @@ import PinkBill from '../../../assets/images/simple-illustrations/simple-illustr | |||
import CreditCardsNew from '../../../assets/images/simple-illustrations/simple-illustration__credit-cards.svg'; | |||
import InvoiceBlue from '../../../assets/images/simple-illustrations/simple-illustration__invoice.svg'; | |||
import LockOpen from '../../../assets/images/simple-illustrations/simple-illustration__lockopen.svg'; | |||
import Lounge from '../../../assets/images/product-illustrations/lounge-top-bg.svg'; |
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.
NAB/nit picks, but 1) I wonder if we should just name this lounge.svg
instead (to stay consistent with how the result of product illustrations are named - doesn't seem like they incorporate placement details into the name), 2) there's some dubious alpha ordering in this import section 😄
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.
SGTM!
src/languages/es.js
Outdated
loungeAccess: 'Acceso al lounge', | ||
headline: 'Podrás acceder a nuestras salas vip exclusivas.', | ||
description: | ||
'La sala vip Expensify es el punto de encuentro entre una “sala vip de aeropuerto de alta gama” y un vibrante “espacio de co-working” optimizado para personas con ideas afines.', |
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.
'La sala vip Expensify es el punto de encuentro entre una “sala vip de aeropuerto de alta gama” y un vibrante “espacio de co-working” optimizado para personas con ideas afines.', | |
'La sala vip Expensify es el punto de encuentro entre una "sala vip de aeropuerto de alta gama" y un vibrante "espacio de co-working" optimizado para personas con ideas afines.', |
@@ -464,6 +464,13 @@ const SettingsModalStackNavigator = createModalStackNavigator([ | |||
}, | |||
name: 'Settings_App_Download_Links', | |||
}, | |||
{ | |||
getComponent: () => { | |||
const SettingsLoungeAccessPage = require('../../../pages/settings/LoungeAccessPage').default; |
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.
Let's nest this under Profile
to follow existing directory organization patterns
const SettingsLoungeAccessPage = require('../../../pages/settings/LoungeAccessPage').default; | |
const SettingsLoungeAccessPage = require('../../../pages/settings/Profile/LoungeAccessPage').default; |
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.
Thoughts on this one @yuwenmemon?
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.
Oh sorry missed this! Sounds good one sec...
const illustrationStyle = props.isSmallScreenWidth | ||
? { | ||
width: props.windowWidth, | ||
height: 0.64 * props.windowWidth, |
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.
NAB but I think we could make this line more self-documenting by using an aspect ratio constant or something?
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.
SGTM!
<MenuItem | ||
key={item.translationKey} | ||
title={props.translate(item.translationKey)} | ||
icon={item.icon} | ||
iconWidth={60} | ||
iconHeight={60} | ||
iconStyles={[styles.mr3, styles.ml3]} | ||
/> |
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'm not sure about the use of the <MenuItem />
component here, since these aren't really menu items (they don't go anywhere). I don't know offhand if we have a more appropriate component to use (like a ListItem
). The main issue I see with it is the hover styles, so maybe you can just pass some custom styles to get around it?
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.
Yeah, maybe no highlighting on hover? Good idea!
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.
Handled this with interactive={false}
!
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.
Oh perfect, easy peas.
} | ||
: {}; | ||
|
||
return ( |
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 we should check if the user has the NVP here as well, and return <NotFoundPage />;
if they don't. Otherwise users who are slightly tech savvy (which, it's SF 😄) could just navigate to the URL directly
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.
Ah yes, great thought!
@@ -118,6 +122,16 @@ function ProfilePage(props) { | |||
onPress={() => Navigation.navigate(ROUTES.SETTINGS_PERSONAL_DETAILS)} | |||
shouldShowRightIcon | |||
/> | |||
{props.user.hasLoungeAccess && ( |
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.
Should we add hasLoungeAccess
to userPropTypes
?
6036e52
to
2dcce6e
Compare
Updated! |
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! 🎉
Question still stands on whether this should get design review though, and one additional question: should we hold merge until after SNH given the feature freeze?
@amyevans looks like we're good to merge here! https://expensify.slack.com/archives/C04QEB4MJEQ/p1687532615305669?thread_ts=1687478534.233329&cid=C04QEB4MJEQ |
Sounds good, are you able to merge? Strangely I'm now seeing this: Asked in Slack too |
Yep seeing the same thing |
It just wasn't meant to be 😏 |
✋ 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 by https://github.com/amyevans in version: 1.3.32-1 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 1.3.32-5 🚀
|
Pullerbearing (@narefyev91)
cc @Beamanator @amyevans
Details
Create a page that will show for users who have lounge access that describes the lounge a bit (but more importantly can be used by our lounge staff to verify a user is a paying member)
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/287746
Tests/QA
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
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android