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.52k stars 2.87k forks source link

[$250] The intro text on More Features stays fixed when it should scroll with the page #49075

Closed m-natarajan closed 3 weeks ago

m-natarajan commented 1 month 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: 9.0.33-1 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: @dubielzyk-expensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1726115971343019

Action Performed:

  1. Go to Profile -> Workspace
  2. Click a workspace -> More features
  3. Observe the intro text

    Expected Result:

    The intro text that says: "use the toggles below to enable..." is fixed when you scroll the page

    Actual Result:

    Intro text should be attached to the content of the page and scroll with the rest of the cards. Also the padding on the right of the text missing. The text is too close to the edge:

    Workaround:

    Unknown

    Platforms:

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

    • [x] Android: Native
    • [x] Android: mWeb Chrome
    • [x] iOS: Native
    • [x] iOS: mWeb Safari
    • [ ] MacOS: Chrome / Safari
    • [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/b1e78d00-750c-4742-849c-45f0a5bb78ad

CleanShot 2024-09-12 at 14 41 24@2x

https://github.com/user-attachments/assets/c84e130f-baf1-4dd0-a176-e88bf11e413a

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021837165152252710251
  • Upwork Job ID: 1837165152252710251
  • Last Price Increase: 2024-09-20
  • Automatic offers:
    • suneox | Reviewer | 104114387
    • truph01 | Contributor | 104114389
Issue OwnerCurrent Issue Owner: @suneox
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @abekkala (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.

ChavdaSachin commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-13 08:13:38 UTC.

Proposal

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

The intro text on More Features stays fixed when it should scroll with the page

What is the root cause of that problem?

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

Optional changes current style of headerText somewhat differs from from other pages headerText so replace [styles.mb4, styles.mt3] with [styles.pb5, styles.pt3] to remove that 4px difference compare to other pages https://github.com/Expensify/App/blob/caf529b46a6d99cab523729bc0ffdad2fbd94e5c/src/pages/workspace/WorkspaceMoreFeaturesPage.tsx#L412 WorkspaceCategoriesPage Reference WorkspaceTagsPage reference

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Nodebrute commented 1 month ago

Proposal

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

The intro text on More Features stays fixed when it should scroll with the page

What is the root cause of that problem?

We are not adding this Text in scrollView. In narrowLayout the header scrolls with the content but here we have not added that functionality. https://github.com/Expensify/App/blob/04762acb53f446708d6f649cbb6b527abbf72889/src/pages/workspace/WorkspaceMoreFeaturesPage.tsx#L412-L414

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

We can create a const getHeaderText to store this

  const getHeaderText = () => (
        <Text style={[styles.ph5, styles.mb4, styles.mt3, styles.textSupporting, shouldUseNarrowLayout ? styles.workspaceSectionMobile : styles.workspaceSection]}>
        {translate('workspace.moreFeatures.subtitle')}
        </Text>
    )

and then outside scrollView here we can add

      {!shouldUseNarrowLayout && getHeaderText()}

and then finally we can change scrollView

         <ScrollView contentContainerStyle={styles.pb2}>
                    {shouldUseNarrowLayout && getHeaderText()}
                    {sections.map(renderSection)}
                    </ScrollView>

Note: We might need some minor styling that can be done in PR.

What alternative solutions did you explore? (Optional)

We can use isSmallScreenWidth instead of shouldUseNarrowLayout

ChavdaSachin commented 1 month ago

Proposal

Updated added additional changes we should make for more consistent user experience.

Reference images for proposal Notice Larger gap between sections on MoreFeaturesPage MoreFeaturesPage WorkflowPage
Screenshot 2024-09-13 at 1 47 22 PM Screenshot 2024-09-13 at 1 48 17 PM
abekkala commented 1 month ago

@Expensify/design I want to confirm that the "expected" above is true:

Intro text should be attached to the content of the page and scroll with the rest of the cards. Also the padding on the right of the text missing. The text is too close to the edge.

Should More Features scroll with the page or remain fixed at the top?

shawnborton commented 1 month ago

The More features part should stay fixed.

abekkala commented 1 month ago

That's what I was assuming Closing

dubielzyk-expensify commented 1 month ago

Why are we closing this? Sorry if I missed something, but this issue isn't fixed is it? To clarify the top header shouldn't scroll, but the text below should:

image

Sorry if I'm missing something though 😅

shawnborton commented 1 month ago

Agree with Jon, I think this is something we need to fix.

abekkala commented 1 month ago

OH! I did misunderstand. I thought you meant the whole "More Features" part stays fixed!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

truph01 commented 1 month ago

Edited by proposal-police: This proposal was edited at 2024-09-21 01:17:03 UTC.

Proposal

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

What is the root cause of that problem?

https://github.com/Expensify/App/blob/04762acb53f446708d6f649cbb6b527abbf72889/src/pages/workspace/WorkspaceMoreFeaturesPage.tsx#L412-L414

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

                <ScrollView contentContainerStyle={[styles.pb2]}>
                    <Text style={[styles.ph5, styles.mb4, styles.mt3, styles.textSupporting, shouldUseNarrowLayout ? styles.workspaceSectionMobile : styles.workspaceSection]}>
                        {translate('workspace.moreFeatures.subtitle')}
                    </Text>
                    {sections.map(renderSection)}
                </ScrollView>

What alternative solutions did you explore? (Optional)

truph01 commented 1 month ago

@dubielzyk-expensify Can you help confirm that: This issue need to be resolved on all platforms, right?

suneox commented 1 month ago

@dubielzyk-expensify Can you help confirm that: This issue need to be resolved on all platforms, right?

I think it should be resolved on all platforms, but we should wait for confirmation.

dubielzyk-expensify commented 1 month ago

Yep. All platforms please

suneox commented 1 month ago

Yep. All platforms please

Based on the confirmation @truph01 proposal LGTM. and I believe the issue The text is too close to the edge has been fixed in #48565.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 1 month ago

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

Nodebrute commented 1 month ago

@dubielzyk-expensify we only scroll the header text in narrow layouts and apply the same approach for the categories page and other workspace pages.

https://github.com/user-attachments/assets/408b5ed7-9d3f-4ebc-9179-59c4f301edf1

And here, only small screen devices are selected.

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

  • [x] Android: Native
  • [x] Android: mWeb Chrome
  • [x] iOS: Native
  • [x] iOS: mWeb Safari
  • [ ] MacOS: Chrome / Safari
  • [ ] MacOS: Desktop
Nodebrute commented 1 month ago

@suneox With the selected proposal, the text will scroll in all cases. We only scroll the header text when we're in narrow layouts.

cc: @dubielzyk-expensify

suneox commented 1 month ago

@suneox With the selected proposal, the text will scroll in all cases. We only scroll the header text when we're in narrow layouts.

@Nodebrute The expected behavior has been confirmed in this comment

Nodebrute commented 1 month ago

Yes, we must fix this on all platforms, but we only scroll the header text in narrow layouts. This is the same approach we use on other workspace pages. Can you check this video https://github.com/Expensify/App/issues/49075#issuecomment-2368158721

cc: @shawnborton @dubielzyk-expensify

dubielzyk-expensify commented 1 month ago

In the wider layout example is it possible to have the intro text scroll, but not the table header?

truph01 commented 1 month ago

In the wider layout example is it possible to have the intro text scroll, but not the table header?

@dubielzyk-expensify Could you provide more details? I don't fully understand what you're referring to.

dubielzyk-expensify commented 1 month ago

My bad. Can we do this?

CleanShot 2024-09-24 at 13 03 23@2x

truph01 commented 1 month ago

I can fix the 'Name-Status' header, but I’m unable to make the 'Get a better overview ...' text scrollable:

https://github.com/user-attachments/assets/ba3e4547-5917-4b80-93dd-492282d93770

truph01 commented 1 month ago

@dubielzyk-expensify I don't see any similarity between the workspace 'More Features' page and other pages like workspace categories, tags, or taxes. Therefore, there’s no reason to require us to apply the same behavior on the 'More Features' page as on those other pages.

So I think we just need to apply the changes to all platforms as you mentioned above:

All platforms please

dubielzyk-expensify commented 1 month ago

Yeah, I think for the pages with tables, let's just leave them as is for now and proceed with the original bug in this issue.

It's still solvable by making a sticky header table when scrolling, but it requires more work

truph01 commented 1 month ago

@srikarparsi C+ has reviewed the proposal. All yours!

dannymcclain commented 1 month ago

I don't see any similarity between the workspace 'More Features' page and other pages like workspace categories, tags, or taxes. Therefore, there’s no reason to require us to apply the same behavior on the 'More Features' page as on those other pages.

Definitely agree with this so I agree with where you all landed. Make More features work how we want it to work and leave the table pages alone for now.

srikarparsi commented 1 month ago

Cool, sounds good. If we want the intro text to scroll up with the page on all platforms then @truph01's proposal looks good to me too

melvin-bot[bot] commented 1 month ago

📣 @suneox 🎉 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 1 month ago

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

truph01 commented 1 month ago

@suneox PR is ready.

suneox commented 4 weeks ago

Checklist

suneox commented 4 weeks ago

@abekkala The PR has been deployed to production for a week, so we can proceed with the payment for this issue.

abekkala commented 3 weeks ago

PAYMENT SUMMARY:

abekkala commented 3 weeks ago

@truph01 and @suneox payments sent and contracts ended - thank you! 🎉