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.49k stars 2.84k forks source link

[$1000] Confirm Task menu item is cut off on touch #18753

Closed kavimuru closed 1 year ago

kavimuru commented 1 year 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!


Action Performed:

  1. Press the Global + (FAB) button
  2. Select Assign Task
  3. Complete the field and press Next
  4. Press Assignee and select any user
  5. Press down the Assignee/Share somewhere
  6. Notice the menu is cut off

Expected Result:

The menu should stay fully visible

Actual Result:

The menu getting cut off

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Version Number: 1.3.12 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

https://github.com/Expensify/App/assets/43996225/bfd3121d-d0ea-4f5d-985b-2be6b8c713f1

Expensify/Expensify Issue URL: Issue reported by: @bernhardoj Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1683727180608179

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a0d40385abc3107d
  • Upwork Job ID: 1656605435245920256
  • Last Price Increase: 2023-05-11
melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

PrashantMangukiya commented 1 year ago

Proposal

Posting proposal early as per new guidelines

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

Confirm Task menu item is cut off on touch (if it consist icon, desc content) for native android etc.

What is the root cause of that problem?

Task menu item rendered via TaskSelectorLink component as shown below: https://github.com/Expensify/App/blob/2fca38477beb91f6cd65b5fc01d1f33f4a1de130/src/components/TaskSelectorLink.js#L50-L60

We can see at line 57 it is using styles.taskSelectorLink that consist below code. https://github.com/Expensify/App/blob/2fca38477beb91f6cd65b5fc01d1f33f4a1de130/src/styles/styles.js#L1685-L1692

We can see height: 42 set at line 1687, so touchable opacity has fix height, So when we touch it will show area with height 42 and other area cut off. We can see it from below debug image with yellow line indicates touchable opacity area. So if we set minHeight: 42 it will generate proper opacity height.

height: 42 minHeight: 42
Before After

So This is the root cause of the problem i.e. when we touch it will cut off the touch.

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

Within src/styles/styles.js we have to change height to minHeight as shown code below.

taskSelectorLink: {
    ...
    //height: 42, // *** OLD CODE ***
    minHeight: 42, // *** NEW CODE ***
    ...
},

As we set minHeight so no need to set linkBottomMargin value conditionally, so we can remove it and set styles.mb2 for TouchableOpacity as shown below:

//const linkBottomMargin = props.icons.length !== 0 ? styles.mb6 : styles.mb2; // *** REMOVE THIS ***

<TouchableOpacity
    //style={[styles.flexRow, styles.taskSelectorLink, linkBottomMargin]} // *** OLD CODE ***
    style={[styles.flexRow, styles.taskSelectorLink, styles.mb2]} // *** NEW CODE ***
    ....
>

This will solve the issue and work as expected on all platform as shown in video for iOS and Android.

What alternative solutions did you explore? (Optional)

None

Results #### Android https://github.com/Expensify/App/assets/7823358/90c15b42-c806-41f2-ac00-f7efdd40d9d7 #### iOS https://github.com/Expensify/App/assets/7823358/b128353b-c3fa-4994-aa12-ff62e38e5e41
hungvu193 commented 1 year ago

Proposal

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

Confirm Task menu item is cut off on touch

What is the root cause of that problem?

We are fixing height for TaskSelectorLink, but in difference devices for example my Pixel 6 emulator, the content of the TaskSelectorLink is overlap the height of 42, that's why when we long press the field the content is cut off. https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/styles/styles.js#L1687

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

Because we are allowing fontScale, that's bad idea to fix the height of a field, in order to fix this problem, we will need to remove the height of TaskSelectorLink.

    taskSelectorLink: {
        alignSelf: 'center',
       // remove the height of TaskSelectorLink
        // height: 42,
        width: '100%',
        padding: 6,
        margin: 3,
        backgroundColor: themeColors.transparent,
    },

We can also replace the TouchableOpacity with PressableWithFeedback to prevent double click bugs (https://github.com/Expensify/App/pull/18122). Since we don't limit the height of TaskSelectorLink we will also need to update the margin of it in here: https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/components/TaskSelectorLink.js#L54 and here: https://github.com/Expensify/App/blob/1d951e6812a30376eaed21ee792c370626e82d89/src/pages/tasks/NewTaskPage.js#L162-L195

What alternative solutions did you explore? (Optional)

None

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Current assignee @flaviadefaria is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

abdulrahuman5196 commented 1 year ago

Thank you for the proposals. Will take a look into it in couple of hours

cdanwards commented 1 year ago

@abdulrahuman5196 just a heads up, this visual bug is addressed in an iteration of the Tasks project here: https://github.com/Expensify/App/pull/18672/files#diff-8ab8ba45ab927f1430b8d1209d187e003a9cdfe476f809e4483ef64cdba253eb

abdulrahuman5196 commented 1 year ago

Thanks for the headsup @cdanwards. Then we can have this issue on hold. Once the https://github.com/Expensify/App/pull/18672 PR is merged, we can re-verify and close out this issue if not reproducible then.

flaviadefaria commented 1 year ago

@stitesExpensify should you put this on hold and switch it to weekly?

abdulrahuman5196 commented 1 year ago

@flaviadefaria The PR is already merged. Will check if the issue is reproducible and update before EOD. We can take action based on that.

abdulrahuman5196 commented 1 year ago

I don't see this issue in main after the PR merge. I think we can close this issue.

https://github.com/Expensify/App/assets/46707890/1d60235c-d279-4dc0-96f6-dfc39af9a1aa

hungvu193 commented 1 year ago

Me too. Just saw that the height is removed from taskSelectorLink which solved this issue 😄

flaviadefaria commented 1 year ago

Perfect so closing this!