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

[HOLD for payment 2023-10-02] [$500] Settings - Help -User can not scroll to to the bottom of the page. #27365

Closed
1 of 6 tasks
izarutskaya opened this issue Sep 13, 2023 · 34 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@izarutskaya
Copy link

izarutskaya commented Sep 13, 2023

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


Action Performed:

1.Login and then open Settings.
2. open help.
3. tap on search icon.
4. type in the search field. I have entered "other" in it,
5. When the search list shows up try to scroll down , it stuck after a little scroll, User can not go though the whole list.

Expected Result:

User should see through whole list.

Actual Result:

User gets stuck while scrolling the list.

Workaround:

Unknown

Platforms:

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

  • Android / native
  • Android / Chrome
  • iOS / native
  • iOS / Safari
  • MacOS / Chrome / Safari
  • MacOS / Desktop

Version Number: v1.3.69-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

Notes/Photos/Videos: Any additional supporting documentation

Screenrecorder-2023-09-02-19-11-49-788.1.mp4
Screen_Recording_20230913_180654_Chrome.mp4

Expensify/Expensify Issue URL:

Issue reported by: @Harshizonnet

Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693664400580059

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a29391c5d4456b92
  • Upwork Job ID: 1701980015607357440
  • Last Price Increase: 2023-09-13
  • Automatic offers:
    • ntdiary | Reviewer | 26722324
    • Harshizonnet | Reporter | 26722326
@izarutskaya izarutskaya added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 13, 2023
@melvin-bot melvin-bot bot changed the title Settings - Help -User can not scroll to to the bottom of the page. [$500] Settings - Help -User can not scroll to to the bottom of the page. Sep 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 13, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

Bug0 Triage Checklist (Main S/O)

  • This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
  • This bug is not a duplicate report (check E/App issues and #expensify-bugs)
    • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
  • This bug is reproducible using the reproduction steps in the OP. S/O
    • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
    • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
  • This issue is filled out as thoroughly and clearly as possible
    • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
  • I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 13, 2023

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

@imranaalam
Copy link


Problem Statement:
Upon logging in and navigating to Settings, when a user accesses the help section and initiates a search (for instance, by entering "other"), the resulting search list becomes unresponsive after a brief scroll. This behavior prevents users from viewing the entirety of the search results. The expected behavior is for users to be able to seamlessly scroll through the full list without any hindrance. As of now, there is no known workaround for this issue.


Root Cause:
On the Android platform, the last item in the search list is partially obscured by the bottom bar, leading users to believe the list is truncated. Verification on Windows Chrome has confirmed that the last item displayed on Android is the final list entry, but it's not fully visible.

image

Proposed Solution:
Introduce additional padding at the end of the search list specifically for the Android platform to ensure that the last item is fully visible above the Android bottom bar.

Alternative Solution:
Re-evaluate the mobile-responsive design elements of the search list on Android. Consider adjusting the overall layout to ensure all items, including the last one, are fully visible.


@imranaalam
Copy link


Problem Statement:
On Android devices, when users access the search functionality and try to view the results, the last item in the list is partially obscured by the Android bottom navigation bar. This behavior makes it appear as though the list is truncated and prevents users from viewing the entire content.

Root Cause:
The web content is rendered in such a way that the total height of the content, including the search results, extends to the bottom edge of the viewport. As a result, the Android bottom navigation bar overlaps with the final items, obscuring them from view.

Proposed Solution:
Adjust the padding or margin at the bottom of the content container (or the search results container) to account for the height of the Android bottom bar. This additional spacing will ensure the last item in the list is fully visible above the Android bottom bar.

#content-area {
    padding-bottom: 60px; /* or an appropriate value based on the average height of the Android bottom bar */
}

Alternative Solutions:

  1. Use a Sticky Footer:
    Design the footer to always stick to the bottom, pushing the content upwards. This ensures that even if users scroll down, the content remains above the bottom bar.

  2. Dynamic Resizing with Event Listeners:
    Attach a resize event listener to the window. If the height changes without a corresponding change in width (indicating the appearance or disappearance of the navigation bar), adjust the content's bottom padding.

  3. Progressive Web App (PWA) Approach:
    If this is a Progressive Web App, consider using a display: standalone or display: fullscreen property in the web app manifest. This ensures the content is displayed in a standalone or fullscreen mode, respectively, reducing browser UI obstructions.

  4. Mobile-specific Styles:
    Use media queries to apply specific styles for mobile devices, ensuring that the content is always above the bottom bar:

@media (max-width: 600px) { /* or other breakpoint for mobile devices */
    #content-area {
        padding-bottom: 60px;
    }
}
  1. Feedback-driven UI Adjustment:
    Integrate a feedback mechanism where users can report UI issues. Use this feedback to gather data on devices where the problem persists and fine-tune the solution for those specific devices.

  2. Test with Different Mobile Browsers:
    The issue might not be consistent across all mobile browsers. Testing on different browsers (like Firefox, Samsung Internet, etc.) can provide insights into whether the problem is browser-specific and if certain browsers have built-in solutions.


@hoangzinh
Copy link
Contributor

hoangzinh commented Sep 13, 2023

Proposal

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

Settings - Help -User can not scroll to to the bottom of the page.

What is the root cause of that problem?

Currently, we fix the max-height for the search page content is 80vh

max-height: 80vh;
overflow-y: scroll;

In Android/small screen device, it's too much space, it makes the content is hidden out of parent element

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

After try a lot of numbers, I found this is the best number that I think we can use to replace for above 80vh

max-height: calc(100vh - 150px);

@include maxBreakpoint($breakpoint-tablet) {
    max-height: calc(100vh - 220px);
}

Result
MacOS
Screenshot 2023-09-14 at 06 41 35

Tablet screen
Screenshot 2023-09-14 at 06 41 44

Mobile screen
Screenshot 2023-09-14 at 06 41 13

@imranaalam
Copy link

imranaalam commented Sep 14, 2023


Problem Statement:
On certain devices, especially Android, users are unable to scroll to the bottom of the search results page in the Expensify Help section. The content is hidden, resulting in a truncated list.

Root Cause:
The max-height for the search page content is set to a static value of 80vh. On some devices, particularly those with prominent browser UI elements or navigation bars, this causes content to overflow and be obscured.

Proposed Solution:

  1. Flexible Container Height with CSS Variables:

    Use CSS variables (custom properties) to dynamically adjust the height based on device characteristics.

    :root {
        --dynamic-max-height: calc(100vh - 150px);
        --tablet-max-height: calc(100vh - 220px);
    }
    
    #sidebar-search {
        max-height: var(--dynamic-max-height);
        overflow-y: scroll;
    }
    
    @media only screen and (max-width: $breakpoint-tablet) {
        #sidebar-search {
            max-height: var(--tablet-max-height);
        }
    }

Alternative Solution:

  1. JavaScript-based Dynamic Adjustment: Detect the presence of an Android navigation bar or any other UI elements and adjust the max-height accordingly.

    const searchBar = document.getElementById('sidebar-search');
    const navBarHeight = getAndroidNavBarHeight();  // This function detects the nav bar height
    
    if (navBarHeight) {
        const adjustedHeight = `calc(100vh - ${navBarHeight}px)`;
        searchBar.style.maxHeight = adjustedHeight;
    }
  2. Viewport Units with Fallback: Utilize a combination of viewport units (vh and vw) along with pixel units for better compatibility.

    #sidebar-search {
        max-height: calc(100vh - 150px);
        max-height: calc(100% - 150px);
        overflow-y: scroll;
    }

@ntdiary
Copy link
Contributor

ntdiary commented Sep 14, 2023

Hi, @imranaalam, thanks for your proposal. Please read our contributor guidelines thoroughly first, and post proposals following the template. Also please hide other proposals to keep the thread clean (you can update the original proposal). 🙂

@suneox
Copy link
Contributor

suneox commented Sep 14, 2023

Proposal

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

Settings - Help -User can not scroll to to the bottom of the page

What is the root cause of that problem?

The search set the fixed height at

max-height: 80vh;

so on a mobile screen the search result height with title + search box over device height

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

Replace the current style

#sidebar-search {
background-color: $color-appBG;
width: 375px;
height: 100vh;
position: fixed;
display: block;
top: 0;
right: 0;
z-index: 2;
}

to

#sidebar-search {
    background-color: $color-appBG;
    width: 375px;
-   height: 100vh;
    position: fixed;
-   display: block;
+   display: flex;
+   flex-direction: column;
+   bottom: 0;
    top: 0;
    right: 0;
    z-index: 2;
}

+#sidebar-search > div:last-child {
+    flex-grow: 1;
+    overflow-y: auto;
+}

+#sidebar-search > div:last-child::-webkit-scrollbar {
+    display: none;
+}

remove 4 lines

max-height: 80vh;
overflow-y: scroll;
-ms-overflow-style: none;
scrollbar-width: none;

Based on the current structure

<div id="sidebar-search" class="sidebar">
<div class="flex searchbar-title-wrapper">
<a id="toggle-search-close">
<img src="/assets/images/back-left.svg" class="base-icon" />
</a>
<h4 class="search-title">Search</h4>
</div>
<!-- As the search box is being managed by Embed script from Google, the way search work is on via on Enter / Search Icon -->
<div class="gcse-searchbox"></div>
<div class="gcse-searchresults"></div>
</div>
</div>

the search result should get all remaining space and shouldn't set a fixed value due to each device having a diffidence ratio so cannot adapt with all devices (when a title or search box update content)
Checkout my proposal structure here on your device

What alternative solutions did you explore? (Optional)

@ntdiary
Copy link
Contributor

ntdiary commented Sep 15, 2023

@suneox, interesting , have you tested your proposal?

@suneox
Copy link
Contributor

suneox commented Sep 16, 2023

@suneox, interesting , have you tested your proposal?

Sure I have tested on real ios and android devices, and I just updated my proposal using id to style instead class after rebase from upstream

27365.mp4

@ntdiary
Copy link
Contributor

ntdiary commented Sep 17, 2023

@suneox, nice! The reason I asked that is because I tried <div class="gcse-searchresults searchresults"></div> before, but the search results failed to load. Additionally, some minor suggestions:

  1. Changing the id or class of the element is not necessary. For example, I was using #sidebar-search > div:last-child during testing. Maybe we can also directly use #___gcse_1 (because I see we already used #___gcse_0 in the _search_bar.scss).
    /* All gsc id & class are Google Search relate gcse_0 is the search bar parent & gcse_1 is the search result list parent */
    #___gcse_0 {
    margin-left: 20px;
    }
  2. The way to hide scrollbars in webkit is slightly different. We also need to apply the same css to #__gsce_1.
    /* Hide the scrollbar */
    .gsc-control-cse::-webkit-scrollbar {
    display: none;
    }

Finally, the flex-based solution makes sense to me, I think we can move forward with this proposal.

🎀 👀 🎀 C+ reviewed

@melvin-bot
Copy link

melvin-bot bot commented Sep 17, 2023

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

@suneox
Copy link
Contributor

suneox commented Sep 17, 2023

@ntdiary Thank you for the suggestion I have updated the current proposals to use #sidebar-search > div:last-child and hide the scroll view to make them consistent with the current code, I don't use #__gsce_1 because it's magical due to code doesn't include this id it will generate on build time, if you need to make consistent I'll be changed to #___gcse_1 .
and the PR is ready to create waiting for the assignment.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 17, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 17, 2023

📣 @ntdiary 🎉 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
Copy link

melvin-bot bot commented Sep 17, 2023

📣 @suneox 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 📖

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 18, 2023
@stephanieelliott stephanieelliott removed their assignment Sep 18, 2023
@suneox
Copy link
Contributor

suneox commented Sep 19, 2023

Thank you @ntdiary @tgolen, I have applied for the job and the PR is ready for review

@melvin-bot
Copy link

melvin-bot bot commented Sep 21, 2023

🎯 ⚡️ Woah @ntdiary / @suneox, great job pushing this forwards! ⚡️

The pull request got merged within 3 working days of assignment, so this job is eligible for a 50% #urgency bonus 🎉

  • when @suneox got assigned: 2023-09-17 23:31:06 Z
  • when the PR got merged: 2023-09-21 02:12:18 UTC

On to the next one 🚀

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Sep 25, 2023
@melvin-bot melvin-bot bot changed the title [$500] Settings - Help -User can not scroll to to the bottom of the page. [HOLD for payment 2023-10-02] [$500] Settings - Help -User can not scroll to to the bottom of the page. Sep 25, 2023
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Sep 25, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.73-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2023-10-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

  • External issue reporter
  • Contributor that fixed the issue
  • Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

  • Merged PR within 3 business days of assignment - 50% bonus
  • Merged PR more than 9 business days after assignment - 50% penalty

@melvin-bot
Copy link

melvin-bot bot commented Sep 25, 2023

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@ntdiary] The PR that introduced the bug has been identified. Link to the PR:
  • [@ntdiary] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@ntdiary] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@ntdiary] Determine if we should create a regression test for this bug.
  • [@ntdiary] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@suneox
Copy link
Contributor

suneox commented Sep 29, 2023

hi @JmillsExpensify I haven't received an offer link, could you please send me an offer link for this job

@melvin-bot melvin-bot bot added Daily KSv2 Overdue and removed Weekly KSv2 labels Oct 1, 2023
@tgolen
Copy link
Contributor

tgolen commented Oct 2, 2023

@JmillsExpensify It looks like this issue is waiting on you. I assume you've been traveling the last couple of days, so I am hopeful this will be addressed soon.

@melvin-bot melvin-bot bot removed the Overdue label Oct 2, 2023
@suneox
Copy link
Contributor

suneox commented Oct 5, 2023

Hi, @JmillsExpensify @ntdiary This is my first task so I don't know the next step what do I have to complete this task? :)

@ntdiary
Copy link
Contributor

ntdiary commented Oct 5, 2023

Hi, @JmillsExpensify @ntdiary This is my first task so I don't know the next step what do I have to complete this task? :)

@suneox, don't worry, when @JmillsExpensify gets back, they will send the offer and complete the payment. 😄


BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@ntdiary] The PR that introduced the bug has been identified. Link to the PR: Improvement: Search Feature for help expensify #24299
  • [@ntdiary] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: Improvement: Search Feature for help expensify #24299 (comment)
  • [@ntdiary] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: No need
  • [@ntdiary] Determine if we should create a regression test for this bug. No need
  • [@ntdiary] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again. No need
  • [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

just a simple layout polish, not a regression, so no need to create regression test.

@JmillsExpensify
Copy link

JmillsExpensify commented Oct 7, 2023

Thanks @ntdiary for completing the BZ checklist!

Payment summary:

@JmillsExpensify
Copy link

Payments sent to issue reporter and contributor+. Offer sent to contributor. Once that is complete, we'll close this issue out.

@ntdiary
Copy link
Contributor

ntdiary commented Oct 8, 2023

Hi, @JmillsExpensify, isn't the urgency bonus 50% (i.e. total amount $750)? Or did I miss something? 😂

@JmillsExpensify
Copy link

Great catch! Yes, you're right. Updated above

@JmillsExpensify
Copy link

Everyone is paid out now. @ntdiary Let's chat in Slack about the extra payment I for C+. We can catch up on the next issue.

@suneox
Copy link
Contributor

suneox commented Oct 8, 2023

Thank @JmillsExpensify I have received payment and thank @ntdiary very much for your support on my first ticket

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants