Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2023-09-21] [$1000] Add avatar details to the Lounge Access Page #24381

Closed thienlnam closed 1 year ago

thienlnam commented 1 year ago

To help identify customers and address #nocreeps violations, we're going to add the avatar to the lounge access page for when users check in.

  1. Update the lounge access page to include the avatar, display name, and login like this mockup

image

Internal issue

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01cb19c0e49400d07e
  • Upwork Job ID: 1689690079128702976
  • Last Price Increase: 2023-08-31
  • Automatic offers:
    • Pujan92 | Contributor | 26439127
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @puneetlath (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Design team member for new feature review - @shawnborton (NewFeature)

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01cb19c0e49400d07e

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @tjferriss (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @puneetlath (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @burczu (External)

thienlnam commented 1 year ago

Accidentally added the 'External' label twice, removed the second C+ assignee - sorry for the noise

samh-nl commented 1 year ago

I'd like to work on this (unsure if this requires a formal proposal).

Pujan92 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add avatar details in the lounge access page

What is the root cause of that problem?

New feature

What changes do you think we should make in order to solve the problem?

  1. Use withCurrentUserPersonalDetails for getting the current user personal details and add session in withOnyx to fetch from Onyx if required to show the email as we do the same for InitialSettingsPage. https://github.com/Expensify/App/blob/cceb862e4566b10cc5afecf1d016af45f87177d6/src/pages/settings/InitialSettingsPage.js#L379

  2. To make the overlay on the Illustration we need to use linear-gradient with colors(transparent, black or appBg). We can use direct css linear-gradient but it will only works for the web and not for native. So we need to use the lib react-native-linear-gradient. This lib is already added in this PR.

For that, we can create a prop called shouldShowGradient in IllustratedHeaderPageLayout and pass value true from LoungeAccessPage. For shouldShowGradient true we need to use LinearGradient by passing the colors which we also configured later if required.

https://github.com/Expensify/App/blob/cceb862e4566b10cc5afecf1d016af45f87177d6/src/components/IllustratedHeaderPageLayout.js#L59-L66 After Lottie,

{shouldShowGradient && <LinearGradient colors={['transparent', themeColors.appBG]} style={[styles.pAbsolute, styles.w100, styles.h100]} />}
  1. Create the flex View with aligned content in the center which will have the avatar, display name and login consisting of required margins/paddings.

  2. To place the content on the Illustration we need to provide a negative margin for the above View.

Result Screenshot 2023-08-11 at 7 43 51 PM
jeet-dhandha commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

What is the root cause of that problem?

What changes do you think we should make in order to solve the problem?

Initial Settings page code (For Profile Render) https://github.com/Expensify/App/blob/ed0e54667ae19c8ad4e73c18ae2bfbc5a0b9d13d/src/pages/settings/InitialSettingsPage.js#L338-L383
Final Output: (Web / iOS) Screenshot 2023-08-11 at 9 41 32 AM Screenshot 2023-08-11 at 9 41 45 AM

Compare Changes Link:

What alternative solutions did you explore? (Optional)

puneetlath commented 1 year ago

@thienlnam do you want to be the CME for this since you're proposing the feature?

thienlnam commented 1 year ago

Sure I can take it

puneetlath commented 1 year ago

Cool, I'll still stay on for the BZ-side.

pradeepmdk commented 1 year ago

can i work on this as this an feature request so i think no need formal proposal.

puneetlath commented 1 year ago

@pradeepmdk you should still make a proposal for how you'll implement it if you'd like the job.

pradeepmdk commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Add avatar details to the Lounge Access Page

What is the root cause of that problem?

new feature request

What changes do you think we should make in order to solve the problem?

https://github.com/Expensify/App/blob/8d84c23ae3a21130a540794104313b2f7e7e6b6a/src/pages/settings/Profile/LoungeAccessPage.js for adding user details in the Illustrated we need to set the parent view should toposition: relative and the user view should be position: absolute in the IllustratedHeaderPageLayout.js we add overlayComponent with position: absolute like footer

  <View style={styles.overscrollSpacer(backgroundColor, windowHeight)} />
                            <View style={[styles.alignItemsCenter, styles.pRelative, styles.justifyContentEnd, StyleUtils.getBackgroundColorStyle(backgroundColor)]}>
                                <Lottie
                                    source={illustration}
                                    style={styles.w100}
                                    autoPlay
                                    loop
                                />
                                {!_.isNull(overlayComponent) && <View style={[styles.loungeAccess]}>
                                    {overlayComponent}
                                </View>}
                            </View>
                            <View style={[styles.pt5]}>{children}</View>
                        </ScrollView>
                        {!_.isNull(footer) && <FixedFooter>{footer}</FixedFooter>}

and we need to create an overlay component with user Details that will pass from https://github.com/Expensify/App/blob/8d84c23ae3a21130a540794104313b2f7e7e6b6a/src/pages/settings/Profile/LoungeAccessPage.js

 <IllustratedHeaderPageLayout
            title={translate('loungeAccessPage.loungeAccess')}
            onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)}
            illustration={LottieAnimations.ExpensifyLounge}
            overlayComponent={<View >
                <Text style={{color: 'green'}}>
                pradeep
            </Text>
            </View>}
        >
Santhosh-Sellavel commented 1 year ago

All the proposals have a general outline, But I would like more details specifically on how we are planning to add the user details above the illustration.

Thanks!

pradeepmdk commented 1 year ago

@Santhosh-Sellavel proposal updated https://github.com/Expensify/App/issues/24381#issuecomment-1673780098

jeet-dhandha commented 1 year ago

@Santhosh-Sellavel Updated Proposal with: (https://github.com/Expensify/App/issues/24381#issuecomment-1673650589)

Please give suggestions if needed.

Pujan92 commented 1 year ago

@Santhosh-Sellavel proposal updated https://github.com/Expensify/App/issues/24381#issuecomment-1673650212.

tjferriss commented 1 year ago

Un-assigning myself as @puneetlath is handling BugZero role.

Santhosh-Sellavel commented 1 year ago

Sorry for the delay here.

After checking the proposals all proposals should do the job, @pradeepmdk proposal has it all required. And I like the idea of overlayComponent for rendering the Avatar and username. Over negative margin idea from other proposals.

cc: @thienlnam Let me know your thoughts

jeet-dhandha commented 1 year ago

@Santhosh-Sellavel Just a small request for you, can you review all other proposals and point out the disadvantage they have over the selected proposal

Pujan92 commented 1 year ago

@Santhosh-Sellavel @pradeepmdk We need the user details to show on the Lottie animation part as well outside of it. I doubt how it can be achieved with Overlay component with user details to place after Lottie code as mentioned in the selected proposal.

Screenshot 2023-08-16 at 4 17 40 PM

I do believe that to achieve the above, negative margin for the View makes sense as we do need a space for the content(making that overflow will overlap with the Lounge access page content and needs adjustment to overcome overlap unnecessarily which doesn't looks correct to me).

pradeepmdk commented 1 year ago

Screenshot 2023-08-16 at 7 20 12 PM @Pujan92 we can do without negative margin

Pujan92 commented 1 year ago

@pradeepmdk I assume you have increased the top margin/padding of the content which starts with "You qualify...". If this is the case then it is the other way around only. So this leads to if we increase more content for the user details you need to adjust or increase more margin/padding as the content is set with the absolute position. Whereas this won't be the situation when we set relative position for the user details content. does this makes sense?

pradeepmdk commented 1 year ago

@Pujan92 I am not using margin/padding for content only using flex for rendering the content

style={[styles.flex1, styles.justifyContentEnd, styles.alignItemsCenter]}
shawnborton commented 1 year ago

Looks like the avatar needs to be a bit bigger too.

pradeepmdk commented 1 year ago

@shawnborton this is bigger image in our app same size in the profile screen avatarSizeLarge: 80,


 avatarLarge: {
        width: variables.avatarSizeLarge,
        height: variables.avatarSizeLarge,
    },
shawnborton commented 1 year ago

Fair point, what if we update those to be 100x100 everywhere?

pradeepmdk commented 1 year ago

let me list out where our these changes reflacted

Pujan92 commented 1 year ago

@pradeepmdk can you plz explain when you use absolute View for the user details part, how the below content goes down with flex without setting any margin/padding between Lottie and content? I would appreciate if you can list down all the suggested changes with code as I still have a doubt about it.(Requesting bcoz my thinking is when the View requires space(height/width) separately we should go with Relative instead absolute)

Screenshot 2023-08-16 at 4 17 40 PM
pradeepmdk commented 1 year ago

@Pujan92 you don't need to move the content to the bottom you can use locations

<LinearGradient
                    locations={[0, 0.73]}
                    colors={['transparent', themeColors.appBG, ]}
                    style={[styles.flex1, styles.justifyContentEnd, styles.alignItemsCenter]}
                >
                   {profile()}
                </LinearGradient>

this will take fill up the Lottie space if we need more space in the bottom we can add in relative to the bottom space

                            <View style={[styles.alignItemsCenter, styles.pRelative, styles.pb8, styles.justifyContentEnd, StyleUtils.getBackgroundColorStyle(backgroundColor)]}>
Pujan92 commented 1 year ago

@Pujan92 you don't need to move the content to the bottom you can use locations locations={[0, 0.73]} <View style={[styles.alignItemsCenter, styles.pRelative, styles.pb8, styles.justifyContentEnd,

Exactly, my point here is that we need to adjust locations and styles.pb8(which you applied for the space I am mentioning) whenever we have a change in the content size of the user details part. Due to this, I am not considering it as a preferable solution. In contrast to above, I am suggesting the overlay only on the Lottie animation part and negative margin for the relative user details View. With that, we don't need to use/adjust locations or extra paddings.

I am not sure whether @Santhosh-Sellavel @thienlnam are following what I am trying to highlight.

Santhosh-Sellavel commented 1 year ago

Thanks, everyone for the active discussion.

Sorry @jeet-dhandha.

I don't see a disadvantage in any of the proposals. We have two similar proposals yours & @Pujan92 only difference is the difference in linear background. I liked the overlay idea In @pradeepmdk's proposal as it comes in handy for other pages too.

Regarding @Pujan92's thoughts

I don't see any negative in using locations.

But setting up the bottom padding seems off to me, Where would this go @pradeepmdk ?

 <View style={[styles.alignItemsCenter, styles.pRelative, styles.pb8, styles.justifyContentEnd, StyleUtils.getBackgroundColorStyle(backgroundColor)]}>
Pujan92 commented 1 year ago

I think the fade-out needs to be applied on only Lottie, otherwise it seems to be like a workaround to me as need to adjust as per the content height. For eg. if in the user details we add some more content then location 0.73 may be starts after the animation, so no effect can be seen on Lottie. Considering the mentioned handy part, I think it won't work consistently when it is used at other places where Lottie's height and the content height get changed as 0.73 seems to be worked only for this scenario.

pradeepmdk commented 1 year ago

@Santhosh-Sellavel without styles.pb8 still working

Screenshot 2023-08-17 at 2 01 58 AM

@Pujan92 I am not adding this linear component to the IllustratedHeaderPageLayout page. overlayComponent comes from LoungeAccessPage and the same for style overlayComponentStyle comes from LoungeAccessPage only. styles.pb8 is still also coming from LoungeAccessPage in overlayComponentStyle for adding space at the bottom. we have already footer component like this only

 <IllustratedHeaderPageLayout
            title={translate('loungeAccessPage.loungeAccess')}
            onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)}
            illustration={LottieAnimations.ExpensifyLounge}
            overlayComponentStyle={[styles.pb8]}
            overlayComponent={
                <LinearGradient
                    locations={[0, 0.73]}
                    colors={['transparent', themeColors.appBG, ]}
                    style={[styles.flex1, styles.justifyContentEnd, styles.alignItemsCenter]}
                >
                   {profile()}
                </LinearGradient>
            }
        >
  !_.isNull(overlayComponent) && <View style={[styles.pAbsolute, styles.w100, styles.h100]}>{overlayComponent}</View>
shawnborton commented 1 year ago

That feels like it doesn't quite match the mockup though - the header area with the lounge illustration feels too short.

pradeepmdk commented 1 year ago

@shawnborton Yes that's why I added styles.pb8 for adding more space for illustration

Santhosh-Sellavel commented 1 year ago

@pradeepmdk where overlayComponentStyle is used?

pradeepmdk commented 1 year ago

@Santhosh-Sellavel here

 <IllustratedHeaderPageLayout
            title={translate('loungeAccessPage.loungeAccess')}
            onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS)}
            illustration={LottieAnimations.ExpensifyLounge}
            illustrationStyle={[{marginBottom: 60}]}
            overlayComponent={
                <LinearGradient
                    locations={[0, 0.8]}
                    colors={['transparent', themeColors.appBG, ]}
                    style={[styles.flex1, styles.justifyContentEnd, styles.alignItemsCenter]}
                >
                   {profile()}
                </LinearGradient>
            }
        >
<Lottie
                                    source={illustration}
                                    style={[styles.w100, ...illustrationStyle]}
                                    autoPlay
                                    loop
                                />
Santhosh-Sellavel commented 1 year ago

Thank you @pradeepmdk!

shawnborton commented 1 year ago

Can you share the latest screenshot of this?

pradeepmdk commented 1 year ago

Screenshot 2023-08-17 at 4 40 39 AM

if we need more space we can increase the padding-bottom

and avatarLarge used those places https://github.com/Expensify/App/blob/b07cbefe04ab2f7d4d815ffa2cb1a082c6ea4950/src/components/AvatarWithImagePicker.js?#L236 https://github.com/Expensify/App/blob/b07cbefe04ab2f7d4d815ffa2cb1a082c6ea4950/src/components/RoomHeaderAvatars.js?#L47

https://github.com/Expensify/App/blob/b07cbefe04ab2f7d4d815ffa2cb1a082c6ea4950/src/pages/DetailsPage.js?#L158

https://github.com/Expensify/App/blob/b07cbefe04ab2f7d4d815ffa2cb1a082c6ea4950/src/pages/ProfilePage.js?#L169

https://github.com/Expensify/App/blob/b07cbefe04ab2f7d4d815ffa2cb1a082c6ea4950/src/pages/settings/InitialSettingsPage.js?#L352 https://github.com/Expensify/App/blob/b07cbefe04ab2f7d4d815ffa2cb1a082c6ea4950/src/pages/workspace/WorkspaceInitialPage.js?#L210

https://github.com/Expensify/App/blob/b07cbefe04ab2f7d4d815ffa2cb1a082c6ea4950/src/pages/workspace/WorkspaceSettingsPage.js

shawnborton commented 1 year ago

Yeah, that doesn't quite match up with the mockup. Can you take another look please and see if we can get that illustration area just slightly taller?

pradeepmdk commented 1 year ago

Screenshot 2023-08-17 at 4 55 50 AM

shawnborton commented 1 year ago

Nice, that feels better!

pradeepmdk commented 1 year ago

can I create the PR?

puneetlath commented 1 year ago

@thienlnam you good with @pradeepmdk moving forward? As @Santhosh-Sellavel recommended here.