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.54k stars 2.89k forks source link

[Search v2.2] Update Trips empty state on Search to avoid double empty state #47937

Closed shawnborton closed 1 month ago

shawnborton commented 2 months ago

Right now when you access the Trips empty state via search, you press a big green button that leads to an RHP view with virtually the same information.

https://github.com/user-attachments/assets/647d89ea-d4a0-4dbd-b781-67032df732aa

Let's just combine everything into the Trips empty state view so that it looks like this: CleanShot 2024-08-23 at 13 23 58@2x

Rather than adding two buttons into the empty state like the RHP does, I'm suggesting we just change the "Book a demo" button to be a link in the text above.

cc @filip-solecki in case this might be interesting to you.

cc @dylanexpensify & @luacmartins to file this one under #44038

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01042577ade1f53d6d
  • Upwork Job ID: 1827040366251521288
  • Last Price Increase: 2024-08-23
  • Automatic offers:
    • DylanDylann | Reviewer | 103693340
Issue OwnerCurrent Issue Owner: @lschurr
melvin-bot[bot] commented 2 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01042577ade1f53d6d

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @lschurr (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 2 months ago

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

ShridharGoel commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-23 18:21:48 UTC.

Proposal

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

Update Trips empty state on Search to avoid double empty state.

What is the root cause of that problem?

New feature.

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

Update the below to use the new UI:

https://github.com/Expensify/App/blob/7a78f634836d9da5d5fbed563d6d63bf7b8ac988/src/pages/Search/EmptySearchView.tsx#L26-L34

In EmptyStateComponent, we can add a new prop called extraInfo or something similar, which can be used for showing the two points about saving money and getting updates.

We can make use of TextLink to have the "Book a demo" clickable text.

We'll not need the RHP now since that UI would be integrated in the empty state itself.

daledah commented 2 months ago

Proposal

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

What is the root cause of that problem?

  1. Update the EmptySearchView to pass the custom title: https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/pages/Search/EmptySearchView.tsx#L30

                    title: translate('travel.title'),
                    titleStyles: {...styles.textAlignLeft},
  2. Update the EmptySearchView to pass the custom subtitle: https://github.com/Expensify/App/blob/0c8455280c738a5db596f34409a0a3177e682e7f/src/pages/Search/EmptySearchView.tsx#L31

    const subtitleComponent = (
        <>
            <Text style={[styles.textSupporting, styles.textNormal]}>
                {'Use Expensify Travel to get the best travel offers and manage all your business expenses in one place. '}
                <TextLink
                    onPress={() => {
                        Linking.openURL(CONST.BOOK_TRAVEL_DEMO_URL);
                    }}
                >
                    {'Book a demo'}
                </TextLink>
                {' to learn more.'}
            </Text>
            <View style={[styles.flex1, styles.flexRow, styles.flexWrap, styles.rowGap4, styles.pt4, styles.pl1]}>
                {tripsFeatures.map(({translationKey, icon}) => (
                    <View
                        key={translationKey}
                        style={styles.w100}
                    >
                        <MenuItem
                            title={translate(translationKey)}
                            icon={icon}
                            iconWidth={variables.menuIconSize}
                            iconHeight={variables.menuIconSize}
                            interactive={false}
                            displayInDefaultIconColor
                            wrapperStyle={[styles.p0, styles.cursorAuto]}
                            containerStyle={[styles.m0, styles.wAuto]}
                            numberOfLinesTitle={0}
                        />
                    </View>
                ))}
            </View>
        </>
    );
const tripsFeatures: FeatureListItem[] = [
    {
        icon: Illustrations.PiggyBank,
        translationKey: 'travel.features.saveMoney',
    },
    {
        icon: Illustrations.Alert,
        translationKey: 'travel.features.alerts',
    },
];

Result

image

DylanDylann commented 2 months ago

@daledah's proposal looks good to me. Please remember to use translation instead of hard-code string

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

Current assignee @luacmartins is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

shawnborton commented 2 months ago

Also I want to add to the solution here: let's update the empty state to use a Lottie animation.

File for that is here: TripsEmptyState-082324-1.json.zip

cc @roryabraham just to double check again, do I need to supply a .lottie file or are the .json files okay?

roryabraham commented 2 months ago

We want .lottie files, which we just get by uploading the json ones to https://app.lottiefiles.com/: https://stackoverflowteams.com/c/expensify/questions/17073

one sec, I'll get that for ya

roryabraham commented 2 months ago

OptimizedAnimations.zip

luacmartins commented 2 months ago

Thanks! Proposal LGTM. Let's do this!

melvin-bot[bot] commented 2 months ago

📣 @DylanDylann 🎉 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 2 months ago

📣 @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

daledah commented 2 months ago

@DylanDylann Currently I'm unable to access Expensify Slack Channel, can you help me ask the team for translations for "Book a demo" and "to learn more"? Seems like we don't have these phrases yet.

DylanDylann commented 2 months ago

Waiting for a confirmation here

luacmartins commented 2 months ago
“Book a demo” = “Reserva una demo”
“to learn more” = “para obtener más información”
daledah commented 2 months ago

@shawnborton Currently on devices with small screens, The display is overflowed because of increase in text contents, and we need to scroll to see the full modal, as shown below:

https://github.com/user-attachments/assets/a44879d0-e73f-41e7-bbb6-c4129d1d2880

Do we still keep this behavior, or we should come up with a new implementation for small devices?

shawnborton commented 2 months ago

I think that's fine, but the modal shouldn't be cut off - we should be able to see the whole thing there.

cc @filip-solecki - I'm not sure how you implemented the height there, but is there some kind of fixed minimum or something? I would think that it would have a min-height but it would also always be tall enough to show all of the inner modal content.

melvin-bot[bot] commented 2 months ago

@luacmartins, @lschurr, @DylanDylann, @daledah Whoops! This issue is 2 days overdue. Let's get this updated quick!

lschurr commented 2 months ago

What's the latest on this one @DylanDylann @daledah?

daledah commented 2 months ago

@lschurr I have a draft PR, but I think we need to address the issue of overflow display on smaller screens, as mentioned in https://github.com/Expensify/App/issues/47937#issuecomment-2314744084 before I can open it.

lschurr commented 2 months ago

Ok, thanks! It seems we're waiting on confirmation from @filip-solecki in that comment

filip-solecki commented 2 months ago

Hi! I think there isn't any max or min height defined for the empty state modal. But looks like there are some styles to change, I guess it might be flex but I'm not sure. Right now I am not able to take a look at this so feel free to find some solutions and ping me, otherwise I can take a look on Friday

lschurr commented 2 months ago

Thoughts @shawnborton @daledah?

shawnborton commented 2 months ago

Let's discuss on the PR? We need to find some kind of solution to the min-height problem.

DylanDylann commented 2 months ago

@filip-solecki Kindly bump https://github.com/Expensify/App/issues/47937#issuecomment-2328144008

On the other hand, @daledah Could you jump into the above problem to unblock the PR? If this is complicated we can raise a bounty later

cc @luacmartins

daledah commented 2 months ago

@DylanDylann I'm working on a solution.

luacmartins commented 1 month ago

PR is merged

melvin-bot[bot] commented 1 month ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

melvin-bot[bot] commented 1 month ago

This issue has not been updated in over 15 days. @luacmartins, @lschurr, @DylanDylann, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

daledah commented 1 month ago

@lschurr This is ready for payment. Please help to sort it out

lschurr commented 1 month ago

Thanks @daledah - Could you point me to the PR that fixed the bug?

daledah commented 1 month ago

These PRs:

lschurr commented 1 month ago

Great, ok! So was there a regression or should this be paid in full? @luacmartins @DylanDylann @daledah

daledah commented 1 month ago

We have a regression

lschurr commented 1 month ago

Payment summary:

daledah commented 1 month ago

@lschurr Offer accepted thanks

lschurr commented 1 month ago

All set!