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.32k stars 2.76k forks source link

[HOLD for payment 2024-02-01] [HOLD for payment 2024-01-25] [$500] Referral page - Animation on Referral page does not move #33046

Closed izarutskaya closed 7 months ago

izarutskaya commented 9 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.12-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause-Internal Team Slack conversation: @

Action Performed:

  1. Click on the FAB.
  2. Click on "Request money"
  3. Enter any amount.
  4. Click on "Next".
  5. Click on "Request money, get $250.".
  6. Click "Learn more"

Expected Result:

Animation on Referral page should move

Actual Result:

Animation on Referral page does not move

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/29c2f12d-9375-4845-b006-cfe0aef9299f

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0192e339c759eff93d
  • Upwork Job ID: 1735235226940305408
  • Last Price Increase: 2023-12-14
  • Automatic offers:
    • akinwale | Reviewer | 28066892
    • BhuvaneshPatil | Contributor | 28066893
melvin-bot[bot] commented 9 months ago

Job added to Upwork: https://www.upwork.com/jobs/~0192e339c759eff93d

melvin-bot[bot] commented 9 months ago

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 9 months ago

Bug0 Triage Checklist (Main S/O)

melvin-bot[bot] commented 9 months ago

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

ZhenjaHorbach commented 9 months ago

Proposal

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

Referral page - Animation on Referral page does not move

What is the root cause of that problem?

We use just icon Not Lottie animation

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

We need to update components and instead icon use Lottie animation

https://github.com/Expensify/App/blob/e81ed8f4c548d0c1675a6d696772a914d115c2b1/src/pages/ReferralDetailsPage.js#L65-L71

For example

 headerContent={ 
                <Lottie
                    source={LottieAnimations.Hands}
                    autoPlay
                    loop
                    style={styles.w100} // styles can be updated
                    webStyle={styles.w100}
                />
}

Or use the implementation from Preferences page

https://github.com/Expensify/App/blob/5c0f3e49129ed3a839523aeab1464e898a02535d/src/pages/settings/Preferences/PreferencesPage.js#L50-L55

For example

        <IllustratedHeaderPageLayout
            title={translate('common.referral')}
            illustration={LottieAnimations.Hands}
            shouldShowBackButton
            backgroundColor={theme.PAGE_THEMES[SCREENS.RIGHT_MODAL.REFERRAL].backgroundColor}
        >

Plus I notice that this file is only place where we use PaymentHands from '@components/Icon/Illustrations';

So we can delete PaymentHands from our project

What alternative solutions did you explore? (Optional)

NA

BhuvaneshPatil commented 9 months ago

Proposal

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

The hands on referral page are not animated

What is the root cause of that problem?

We have used SVG icon instead of lottie animation. https://github.com/Expensify/App/blob/5c0f3e49129ed3a839523aeab1464e898a02535d/src/pages/ReferralDetailsPage.js#L63-L74

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

What alternative solutions did you explore? (Optional)

We shall use IllustratedHeaderPageLayout component with LottieAnimations.Hands to wrap the content, similar to what we have used in WorkspaceListPage IllustratedHeaderPageLayout wraps the HeaderPageLayout so we shall consider the component that is built for specific purpose.

melvin-bot[bot] commented 9 months ago

@akinwale, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mallenexpensify commented 8 months ago

@akinwale please review the above proposals

akinwale commented 8 months ago

This is a straightforward change and we can go with @ZhenjaHorbach's proposal here, considering the chronological order of the posted proposals.

πŸŽ€ πŸ‘€ πŸŽ€ C+ reviewed.

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @rlinoz, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

BhuvaneshPatil commented 8 months ago

Hello @akinwale Can you please check the proposal that was posted early, because as. far as I remember when I posted proposal selected proposal didn't mention about IllustratedHeaderPageLayout. They simply mentioned about changing value of headerContent prop. And I agree the fix is simple so there should be emphasis on implementation details. Please re-consider. And looking at the edits of selected proposal, it wasn't well prepared at the begin.

akinwale commented 8 months ago

@BhuvaneshPatil

Looking at the timestamps, it appears that both proposals were being edited simultaneously, which makes it difficult to determine who arrived at the correct solution.

In the future, it's best to follow the guidelines when making updates to proposals by creating a corresponding Updated Proposal post, so that it's easier to follow.

I will leave this up to the internal engineer to decide.

BhuvaneshPatil commented 8 months ago

@akinwale I understand it's quite hectic to go through timestamps. If we consider usage of IllustratedHeaderPageLayout. Then my proposal was the first one to propose (in the initial version, without edit).

Here are the details - For selected proposal - Original proposal posted at 3:25, it didn't mention about what component to use. Then It was updated frequently (once a minute) for 3-4 times. At 3:35 it mentioned about IllustratedHeaderPageLayout.

My Proposal - First posted at 3:28, It mentioned root cause and Idea of using IllustratedHeaderPageLayout. Edited at same minute (grammar). at 3:46 Added explanation for using IllustratedHeaderPageLayout.

Sorry for being pushy, but this has happened with me before, the frequently edited proposal was selected just because it was posted earlier(incomplete).

@mallenexpensify @rlinoz Please consider these details before finalising.

rlinoz commented 8 months ago

Looking at the timestamps the first proposal to mention IllustratedHeaderPageLayout is indeed from @BhuvaneshPatil , @akinwale are you ok if we go with his proposal?

akinwale commented 8 months ago

Looking at the timestamps the first proposal to mention IllustratedHeaderPageLayout is indeed from @BhuvaneshPatil, @akinwale are you ok if we go with his proposal?

@rlinoz Yes, we can move forward with @BhuvaneshPatil's proposal since it was first with the solution chronologically.

melvin-bot[bot] commented 8 months ago

πŸ“£ @akinwale πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 8 months ago

πŸ“£ @BhuvaneshPatil πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

rlinoz commented 8 months ago

Thanks, assigned @BhuvaneshPatil to continue!

BhuvaneshPatil commented 8 months ago

@rlinoz @mallenexpensify Is this size works for lottie animation?

Screenshot 2023-12-22 at 4 04 29β€―PM
rlinoz commented 8 months ago

@BhuvaneshPatil I am not sure what you mean

BhuvaneshPatil commented 8 months ago
Screenshot 2023-12-23 at 6 40 34β€―PM

After changes, this is how page looks.

melvin-bot[bot] commented 8 months ago

Auto-assign attempt failed, all eligible assignees are OOO.

mallenexpensify commented 8 months ago

Auto-assign attempt failed, all eligible assignees are OOO.

hahhaaha, Happy Holidays! Let's wait a week then I'll assign someone from design to review. Cool?

rlinoz commented 8 months ago

Waiting holidays to re-add design label.

melvin-bot[bot] commented 8 months ago

@akinwale @rlinoz @mallenexpensify @BhuvaneshPatil this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 8 months ago

@akinwale, @rlinoz, @mallenexpensify, @BhuvaneshPatil Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 8 months ago

@akinwale, @rlinoz, @mallenexpensify, @BhuvaneshPatil Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 8 months ago

Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.

mallenexpensify commented 8 months ago

@dannymcclain πŸ‘€ please on the above for the lottie animation https://github.com/Expensify/App/issues/33046#issuecomment-1867554167

dannymcclain commented 8 months ago

Hmm that does not look right. I think the latest mock for this page looks like this: CleanShot 2024-01-03 at 08 17 48@2x

But I'm not 100% sure if we're wanting to replace the illustration Jon has in Figma with the animation. But either way, the illustration area looks much too tall and awkwardly spaced.

@shawnborton @dubielzyk-expensify do either of you have any added context on this that I may have missed?

shawnborton commented 8 months ago

I think this is not actually a bug, as I think this was never intended to be animated. That being said, we could totally animate it.

I think we just need to update the illustration as Jon had mocked up (what Danny is showing above).

Either way, I don't think we should open up a new bug for proposals. This should be a follow up issue for @rezkiy37

More context in Slack here

dubielzyk-expensify commented 8 months ago

There wasn't any intention to animate it. Yeah, we'll just need the updated illustration

melvin-bot[bot] commented 8 months ago

@akinwale @rlinoz @dannymcclain @mallenexpensify @BhuvaneshPatil this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

mallenexpensify commented 8 months ago

I don't think we should open up a new bug for proposals. This should be a follow up issue for @rezkiy37

@shawnborton , we've already hired/assigned contributors and have an accepted proposal so it might be easiest and best to have them finish this than to ask @rezkiy37 to do so. Either way, I think we need an updated file to use.

shawnborton commented 8 months ago

Works for me. Perhaps @dubielzyk-expensify can prep the new illustration to be used here.

dubielzyk-expensify commented 8 months ago

This should work, but let me know if you need any tweaks

ill.svg.zip ill

rezkiy37 commented 8 months ago

Hey, guys! I am back. @shawnborton @mallenexpensify do you still need any my help here?

shawnborton commented 8 months ago

Nice, while we're at it, I think we also need to fix the text size below the headline. @dubielzyk-expensify can you provide a new mockup of what we want this to look like, and then @rezkiy37 can follow up and implement the adjustments? Thanks!

dubielzyk-expensify commented 8 months ago

Desktop and mobile mocks: image image

shawnborton commented 8 months ago

Lovely, thank you!

rezkiy37 commented 8 months ago

Well, @dubielzyk-expensify can I ask you to share a little bit more info about the text size below the headline? I believe my eyes, but numbers are more accurate πŸ™‚

@shawnborton, can I use this issue for this minor fix, or will we create another one?

rezkiy37 commented 8 months ago

Hi, I’m Michael (Mykhailo) from Callstack and I would like to work on this issue.

dubielzyk-expensify commented 8 months ago

Below are my measurements... CleanShot 2024-01-08 at 20 52 08@2x

...but, I'd lean on replicating them spacing and variables that's used on our "Workspaces" empty state as that has the sizes we wanna replicate: CleanShot 2024-01-08 at 20 53 24@2x

You can get to that screen by going to Settings/Profile icon -> Workspaces (without any workspaces set up)

shawnborton commented 8 months ago

I think the text size should actually be 15px, just our normal text size we use throughout the app.

Otherwise yup, the idea is to match the workspaces page as much as possible given the layouts are nearly identical.

rezkiy37 commented 8 months ago

@shawnborton @dubielzyk-expensify I've tried to apply the same font styles and spacings. Please check it out:

Dark theme Dark
Light theme Light
shawnborton commented 8 months ago

Just want to confirm that there is 20px padding both horizontally and at the top of the Title + paragraph block? It looks like the padding on the left side of the text is smaller in the left screenshot.

For the illustration, @dubielzyk-expensify mind packaging up an svg for us when you get a minute? Thanks!

dannymcclain commented 8 months ago

Good catch Shawn, the padding does look a teeny bit off. Also, I think Jon already posted the SVG.

shawnborton commented 8 months ago

Ah sweet, thanks for linking that!

rezkiy37 commented 8 months ago

By the mockup, there is no any gap between the icon and content below. However, @dubielzyk-expensify shared with us here the icon has a space at the bottom. So can I ask you please to send one more icon without such paddings?

Screenshot 2024-01-08 at 15 29 55
dannymcclain commented 8 months ago

@rezkiy37 Preview/Finder does not do a good job at previewing the actual bounds of an SVG. I opened the SVG in illustrator to verify the bounding box and here is what I see: CleanShot 2024-01-08 at 12 15 12@2x

In contrast, here is what I see in Finder: CleanShot 2024-01-08 at 12 17 29@2x

And what I see in Quick Look is the same as you.

So basically, don't trust what you see in Quick Look, Finder, or Preview AT ALL for SVGs πŸ˜‚. The file looks like it's good to go to me.