Skip to content
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

[$250] Web - LHN - LHN shows empty without any placeholder #54569

Open
1 of 8 tasks
vincdargento opened this issue Dec 25, 2024 · 43 comments
Open
1 of 8 tasks

[$250] Web - LHN - LHN shows empty without any placeholder #54569

vincdargento opened this issue Dec 25, 2024 · 43 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Design External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@vincdargento
Copy link

vincdargento commented Dec 25, 2024

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: v9.0.78-2
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team

Action Performed:

  1. Login into same account in 2 tabs.
  2. Make sure LHN doesn't have any pinned or unread chats.
  3. From first tab, go any invalid report route (ie: https://staging.new.expensify.com/r/abcxyz)
  4. From second tab, turn on the #focus mode. Go back to the first tab.
  5. Notice the the main LHN screen is empty without any placeholder.

Expected Result:

If LHN is empty, we should show the same empty state we use on mobile. It should look like this:
image

Note that we also want to slightly update the text below the "Hmm... it's not here" text to use our textSupporting color, with a small copy update as well. We also need to make sure there is a search icon in the top right that launches the search router.

Actual Result:

There's no placeholder and it shows empty screen.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

bug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021873766942117852803
  • Upwork Job ID: 1873766942117852803
  • Last Price Increase: 2025-01-06
  • Automatic offers:
    • truph01 | Contributor | 105640342
@vincdargento vincdargento added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 25, 2024
Copy link

melvin-bot bot commented Dec 25, 2024

Triggered auto assignment to @bfitzexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@truph01
Copy link
Contributor

truph01 commented Dec 25, 2024

Edited by proposal-police: This proposal was edited at 2025-01-08 12:42:29 UTC.

Proposal

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

  • There's no placeholder and it shows empty screen.

What is the root cause of that problem?

  • We initially implemented logic to add Concierge to the LHN when the report list was empty in PR #19945. However, this logic was removed in PR #37361, where we opted to show an empty state view when no reports were available in the LHN. This approach works as intended for smaller screens.

  • On wide screens, the assumption was that there would always be an open chat, rendering the empty state view unnecessary. However, this issue highlights a scenario where no reports are displayed in the LHN on wide screens, exposing a gap in the current logic.

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

  • We can remove the shouldUseNarrowLayout in:

const shouldShowEmptyLHN = shouldUseNarrowLayout && data.length === 0;

so the empty state view can be displayed in all devices, not only small screens.

  • Then, we need to update the text below the "Hmm... it's not here" text to use our textSupporting color, with a small copy update as well. We also need to make sure there is a search icon in the top left that launches the search router. To do it, with the not here page in the ReportScreen, we use:
                <HeaderWithBackButton
                    shouldDisplaySearchRouter
                />

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  • Add a unit test to verify that the LHNOptionsList renders the empty state when the data is empty: The test will involve rendering the sidebar, mocking the reports data to ensure no reports are displayed in the sidebar, and then verifying that the empty state view appears as expected
        it('display empty state', () => {
            const reportCollectionDataSet: ReportCollectionDataSet = {};
            LHNTestUtils.getDefaultRenderedSidebarLinks();
            return (
                waitForBatchedUpdates()
                    // When Onyx is updated to contain that data and the sidebar re-renders
                    .then(() =>
                        Onyx.multiSet({
                            [ONYXKEYS.NVP_PRIORITY_MODE]: CONST.PRIORITY_MODE.GSD,
                            [ONYXKEYS.PERSONAL_DETAILS_LIST]: LHNTestUtils.fakePersonalDetails,
                            [ONYXKEYS.IS_LOADING_APP]: false,
                            ...reportCollectionDataSet,
                        }),
                    )

                    .then(() => {
                        expect(screen.getByText(Localize.translateLocal('common.emptyLHN.title'))).toBeOnTheScreen();
                    })
            );
        });

What alternative solutions did you explore? (Optional)

  • We can restore the logic from the add Concierge to the LHN PR, but now, we will apply it only on wide screens to maintain consistency with the approach outlined in the Empty state UI for LHN PR. In detail, the condition to add the concierge report will be updated from _.isEmpty(reportsToDisplay) to _.isEmpty(reportsToDisplay) && !shouldUseNarrowLayout

Result

image

@truph01
Copy link
Contributor

truph01 commented Dec 25, 2024

@bfitzexpensify

We initially implemented logic to add Concierge to the LHN when the report list was empty in PR #19945, which aligns with the expected behavior in this issue. However, that logic was removed in PR #37361, where we shifted to displaying an empty state view when no reports were available in the LHN. While this works as intended for smaller screens, we didn't implement it in wide screen.

Would you like to display the empty state view on wide screens as well, or should we still keep the expected behavior outlined in the OP?

@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2024
@bfitzexpensify bfitzexpensify added the External Added to denote the issue can be worked on by a contributor label Dec 30, 2024
@melvin-bot melvin-bot bot changed the title Web - LHN - LHN shows empty without any placeholder [$250] Web - LHN - LHN shows empty without any placeholder Dec 30, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 30, 2024
@melvin-bot melvin-bot bot removed the Overdue label Dec 30, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

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

@bfitzexpensify
Copy link
Contributor

Would you like to display the empty state view on wide screens as well, or should we still keep the expected behavior outlined in the OP?

Let's go with the former for consistency

@melvin-bot melvin-bot bot added the Overdue label Jan 2, 2025
@truph01
Copy link
Contributor

truph01 commented Jan 3, 2025

If we apply an empty state view to the wide screen, it will look like this:

image

Copy link

melvin-bot bot commented Jan 3, 2025

@allroundexperts, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Jan 3, 2025

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

@melvin-bot melvin-bot bot removed the Overdue label Jan 3, 2025
@bfitzexpensify
Copy link
Contributor

Best to get some Design eyes on this.

Summary from this comment - #54569 (comment):

We initially implemented logic to add Concierge to the LHN when the report list was empty in PR #19945, which aligns with the expected behavior in this issue. However, that logic was removed in PR #37361, where we shifted to displaying an empty state view when no reports were available in the LHN. While this works as intended for smaller screens, we didn't implement it in wide screen.

@shawnborton
Copy link
Contributor

Before we dive too far into this, do we all generally agree that even making this scenario happen is a pretty extreme edge case as it requires two separate tabs to be open and it requires you to navigate to an invalid route?

That being said, I don't think I would spend too much time on this, I think for wide screens I would just select the Concierge chat or last chat that was accessed in this particular case.

cc @Expensify/design for thoughts.

@dubielzyk-expensify
Copy link
Contributor

Before we dive too far into this, do we all generally agree that even making this scenario happen is a pretty extreme edge case as it requires two separate tabs to be open and it requires you to navigate to an invalid route?

Very much agree. Given we also pin chats by default it feels like a very very uncommon use case.

@melvin-bot melvin-bot bot added the Overdue label Jan 6, 2025
@truph01
Copy link
Contributor

truph01 commented Jan 6, 2025

t requires two separate tabs to be open

No. We didn't need this step to reproduce the bug. Just need to follow these steps:

  1. Login into new account
  2. Make sure LHN doesn't have any pinned or unread chats.
  3. Turn on the #focus mode
  4. Go to invalid route, such as https://staging.new.expensify.com/r/abcxyz
  5. Notice the the main LHN screen is empty without any placeholder.

Additionally, we already have an empty state view for smaller screens. Applying it to wide screens would only require a few lines of code (2–3 lines).

@melvin-bot melvin-bot bot removed the Overdue label Jan 7, 2025
@shawnborton
Copy link
Contributor

That being said, we need to find a way to actually get the Search icon into that mock above. It should probably be in the top right on desktop like it is on a view that actually has a chat.

@shawnborton
Copy link
Contributor

And while we're at it, we should update the paragraph text in this empty state (and any others that use this same template) to use our textSupporting color (a grayish color):
CleanShot 2025-01-07 at 10 28 43@2x

So it matches what we do on the left:
CleanShot 2025-01-07 at 10 29 09@2x

Copy link

melvin-bot bot commented Jan 7, 2025

@shawnborton, @allroundexperts, @bfitzexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

@dannymcclain
Copy link
Contributor

Ok yeah I'm down with that. Agree with everything Shawn said.

For the it's not here screen, do they really not have access to it or is it just a bad path? Do we know? Should we consider updating that text to something like this?

Hmm... it's not here
That chat doesn't exist or you don't have access to it. Try using search to find a chat.

Or something? 🤷‍♂️

@shawnborton
Copy link
Contributor

I think that's a nice suggestion.

@dubielzyk-expensify
Copy link
Contributor

I like that suggestion a lot too. Also agree with you Shawn that I don't think it's that bad with two "empty states" cause it's a reflection of focus mode and an error.

@truph01
Copy link
Contributor

truph01 commented Jan 8, 2025

Could someone update the original post to include the expected UI based on the lengthy discussion? I believe this would be beneficial for ensuring clarity and alignment among all contributors, making it easier to reference and implement the agreed-upon solution.

Copy link

melvin-bot bot commented Jan 8, 2025

@shawnborton @allroundexperts @bfitzexpensify 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!

@shawnborton
Copy link
Contributor

I updated the original post with the expected screenshot and some other notes. Let's start getting proposals for this cc @bfitzexpensify @allroundexperts.

@truph01
Copy link
Contributor

truph01 commented Jan 8, 2025

Proposal updated

  • Added test branch and result

@dannymcclain
Copy link
Contributor

I updated the original post with the expected screenshot and some other notes

@shawnborton I think your text on the error page is a little darker than text-supporting? But it's noted correctly and everything else looks great.

@shawnborton
Copy link
Contributor

Oopsies, fixed!

@allroundexperts
Copy link
Contributor

@truph01's proposal looks good to me. The RCA is clear enough and the proposed solution seems fine to me as well. Let's go with them.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Jan 8, 2025

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

@arosiclair
Copy link
Contributor

We also need to make sure there is a search icon in the top left that launches the search router.

@truph01 can you expand on how you plan on adding the search icon to the Not Found page? Also it should be in the top right.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

This section isn't completed

@truph01
Copy link
Contributor

truph01 commented Jan 9, 2025

can you expand on how you plan on adding the search icon to the Not Found page? Also it should be in the top right.

With the not here page in the ReportScreen, I use:

                <HeaderWithBackButton
                    shouldDisplaySearchRouter
                />

The result and the detailed code changes are included in my proposal

This section isn't completed

I believe this is a UI-related issue, so testing may not be necessary.

@arosiclair
Copy link
Contributor

Thanks, can you update your proposal with those details? Please include explanations like that for your changes in your proposals. Do not just link to a branch.

I believe this is a UI-related issue, so testing may not be necessary.

The UI can be tested. Let's add a unit test to verify the the LHNOptionsList renders the empty state when data is empty.

@truph01
Copy link
Contributor

truph01 commented Jan 9, 2025

@arosiclair I updated my proposal.

@allroundexperts
Copy link
Contributor

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

@arosiclair Are unit tests considered part of the automated tests? I was under the impression that automated tests are done using a UI tool like Selenium.

@arosiclair
Copy link
Contributor

@arosiclair I updated my proposal.

Thanks 👍

@arosiclair Are unit tests considered part of the automated tests? I was under the impression that automated tests are done using a UI tool like Selenium.

"Automated tests" are referring to unit tests with Jest. We don't use any tools like Selenium. The new guidelines are here.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 10, 2025
Copy link

melvin-bot bot commented Jan 10, 2025

📣 @truph01 🎉 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Jan 13, 2025
@truph01
Copy link
Contributor

truph01 commented Jan 14, 2025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Design External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

8 participants