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

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

Closed
2 of 6 tasks
kavimuru opened this issue May 10, 2023 · 16 comments
Closed
2 of 6 tasks

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

kavimuru opened this issue May 10, 2023 · 16 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@kavimuru
Copy link

kavimuru commented May 10, 2023

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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?

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

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

Screen.Recording.2023-05-10.at.21.55.07.mov

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
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 10, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 10, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 10, 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

@PrashantMangukiya
Copy link
Contributor

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:

const TaskSelectorLink = (props) => {
const shortenedText = props.text.length > 35 ? `${props.text.substring(0, 35)}...` : props.text;
const displayNameStyle = StyleUtils.combineStyles(styles.optionDisplayName, styles.pre);
const alternateTextStyle = StyleUtils.combineStyles(styles.sidebarLinkText, styles.optionAlternateText, styles.textLabelSupporting, styles.pre);
const linkBottomMargin = props.icons.length !== 0 ? styles.mb6 : styles.mb2;
return (
<TouchableOpacity
style={[styles.flexRow, styles.taskSelectorLink, linkBottomMargin]}
onPress={props.onPress}
disabled={props.disabled}
>

We can see at line 57 it is using styles.taskSelectorLink that consist below code.

App/src/styles/styles.js

Lines 1685 to 1692 in 2fca384

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

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

Android.mp4

iOS

iOS.mov

@hungvu193
Copy link
Contributor

hungvu193 commented May 11, 2023

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.

height: 42,

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 (#18122).
Since we don't limit the height of TaskSelectorLink we will also need to update the margin of it in here:

const linkBottomMargin = props.icons.length !== 0 ? styles.mb6 : styles.mb2;

and here:
<View style={styles.mb5}>
<TaskSelectorLink
text={props.task.title}
onPress={() => Navigation.navigate(ROUTES.NEW_TASK_TITLE)}
label="newTaskPage.title"
/>
</View>
<View style={styles.mb5}>
<TaskSelectorLink
text={props.task.description}
onPress={() => Navigation.navigate(ROUTES.NEW_TASK_DESCRIPTION)}
label="newTaskPage.description"
/>
</View>
<View style={styles.mb5}>
<TaskSelectorLink
icons={assignee.icons}
text={assignee.displayName}
alternateText={assignee.subtitle}
onPress={() => Navigation.navigate(ROUTES.NEW_TASK_ASSIGNEE)}
label="newTaskPage.assignee"
/>
</View>
<View style={styles.mb5}>
<TaskSelectorLink
icons={shareDestination.icons}
text={shareDestination.displayName}
alternateText={shareDestination.subtitle}
onPress={() => Navigation.navigate(ROUTES.NEW_TASK_SHARE_DESTINATION)}
label="newTaskPage.shareSomewhere"
isShareDestination
disabled={Boolean(props.task.parentReportID)}
/>
</View>

What alternative solutions did you explore? (Optional)

None

@flaviadefaria flaviadefaria added the External Added to denote the issue can be worked on by a contributor label May 11, 2023
@melvin-bot melvin-bot bot changed the title Confirm Task menu item is cut off on touch [$1000] Confirm Task menu item is cut off on touch May 11, 2023
@melvin-bot
Copy link

melvin-bot bot commented May 11, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 11, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented May 11, 2023

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

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

melvin-bot bot commented May 11, 2023

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

@abdulrahuman5196
Copy link
Contributor

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

@cdanwards
Copy link
Contributor

@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
Copy link
Contributor

Thanks for the headsup @cdanwards. Then we can have this issue on hold. Once the #18672 PR is merged, we can re-verify and close out this issue if not reproducible then.

@melvin-bot melvin-bot bot added the Overdue label May 15, 2023
@flaviadefaria
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label May 15, 2023
@abdulrahuman5196
Copy link
Contributor

@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
Copy link
Contributor

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

Screen.Recording.2023-05-16.at.7.59.06.PM.mov

@hungvu193
Copy link
Contributor

hungvu193 commented May 16, 2023

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

@flaviadefaria
Copy link
Contributor

Perfect so closing this!

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. Daily KSv2 External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

7 participants