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-18] [$500] Web - Cursor Remains as Hand Icon on Checkbox in "Paid By" Section During Split Bill #28064

Closed
1 of 6 tasks
kbecciv opened this issue Sep 23, 2023 · 45 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

@kbecciv
Copy link

kbecciv commented Sep 23, 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. Click on FAB and select "Send Message"
  2. Add two emails and create a group with the two emails
  3. Inside the chat, click on the plus sign and select "Split Bill"
  4. Add the desired amount and click "Next"
  5. Hover over email (cursor is disabled)
  6. Hover over the checkbox (cursor changes to hand icon, but remains delectable)

Expected Result:

When hovering over the checkbox in the "Paid By" section during split bill, the cursor should be disabled, just like when hovering over the associated email.

Actual Result:

While hovering over the email, the cursor is properly disabled. However, when hovering over the checkboxes, the cursor changes to a hand icon during split bill.

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: 1.3.73.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

screen-capture.mp4
Recording.4714.mp4

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e8f7be88bc37af93
  • Upwork Job ID: 1705585682941407232
  • Last Price Increase: 2023-09-30
  • Automatic offers:
    • aimane-chnaif | Reviewer | 27031408
    • esh-g | Contributor | 27031411
    • tewodrosGirmaA | Reporter | 27031412
@esh-g
Copy link
Contributor

esh-g commented Sep 23, 2023

Proposal

Please re-state the issue

Cursor is not disabled in SelectCircle when the option is disabled.

What is the root cause of this issue?

The reason that the cursor is disabled on the rest of the row option is because we are passing the isDisabled prop to the PressableWithFeedback component wrapping the whole OptionRow.

disabled={this.state.isDisabled}

We are not passing the isDisabled prop to the PressableWithFeedback which wraps the SelectCircle component here:

<PressableWithFeedback
onPress={() => this.props.onSelectedStatePressed(this.props.option)}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
accessibilityLabel={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
>
<SelectCircle isChecked={this.props.isSelected} />
</PressableWithFeedback>

What changes do you think should be made to fix this problem?

We should pass the isDisabled prop the same way we pass it to the root PressableWithFeedback of the OptionRow as mentioned in the root cause.

Optional:
We can also pass isDisabled prop to the Button element (the one which says 'split' or 'add to group' in the options) for consistency but since I don't see a case where the row with a button will be disabled, this is not required.

What other alternative options did you explore? (Optional)

@kbecciv kbecciv 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 23, 2023
@c3024
Copy link
Contributor

c3024 commented Sep 23, 2023

Proposal

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

The paid by row Pressable is disabled but hand cursor is shown on the selectRow even though it is unclickable

What is the root cause of that problem?

We are not disabling this Pressable

<PressableWithFeedback
onPress={() => this.props.onSelectedStatePressed(this.props.option)}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
accessibilityLabel={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
>
<SelectCircle isChecked={this.props.isSelected} />
</PressableWithFeedback>

like we do here
disabled={this.state.isDisabled}

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

We should use the same disabling here

accessibilityRole={CONST.ACCESSIBILITY_ROLE.CHECKBOX}

for this Pressable as well.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot changed the title Web - Cursor Remains as Hand Icon on Checkbox in "Paid By" Section During Split Bill [$500] Web - Cursor Remains as Hand Icon on Checkbox in "Paid By" Section During Split Bill Sep 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 23, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 23, 2023

Triggered auto assignment to @muttmuure (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 23, 2023
@melvin-bot
Copy link

melvin-bot bot commented Sep 23, 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 23, 2023

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

@melvin-bot
Copy link

melvin-bot bot commented Sep 23, 2023

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

@DylanDylann
Copy link
Contributor

DylanDylann commented Sep 23, 2023

Proposal

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

Cursor Remains as Hand Icon on Checkbox in "Paid By" Section During Split Bill

What is the root cause of that problem?

We are using disable props for OptionRow

disabled={this.state.isDisabled}

But we don't do that for the checkbox

<PressableWithFeedback
onPress={() => this.props.onSelectedStatePressed(this.props.option)}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
accessibilityLabel={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
>
<SelectCircle isChecked={this.props.isSelected} />
</PressableWithFeedback>
)}

It causes the consistency

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

First, Let's see the transaction report, If we hover detail transactions, it display default cursor
Screenshot 2023-09-24 at 01 46 52

and cancelled task case: https://expensify.slack.com/archives/C049HHMV9SM/p1695024078116809?thread_ts=1694080698.301029&cid=C049HHMV9SM
In this case, we also should use the default cursor instead of disabled cursor for all option row (include checkbox) if want to disable that.

To do that:

  1. remove disable prop here
    disabled={this.state.isDisabled}
  2. replace this style
 (!this.props.onSelectRow || this.props.isDisabled) ? styles.cursorDefault : null,

in

!this.props.onSelectRow && !this.props.isDisabled ? styles.cursorDefault : null,

and add in
<PressableWithFeedback

What alternative solutions did you explore? (Optional)

@JawadSadiq01
Copy link

JawadSadiq01 commented Sep 24, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.
Cursor Remains as Hand Icon on Checkbox in "Paid By" Section During Split Bill

What is the root cause of that problem?
SelectCircle is a re-usable common component. It's only working with one prop name as isChecked. There is nothing else, that tells either its disable or clickable.

What changes do you think we should make in order to solve the problem?
You just have to add one more prop into SelectCircle that tells, either its active or disabled.

Do the followings;

1. You have to pass a disable prop with a particular name(I named that isDisabled) from OptionsRow Component(line#291).

<SelectCircle isChecked={this.props.isSelected} isDisabled={this.props.isDisabled} />

image

2. Apply this isDisable prop inside the SelectCircle Component

import React from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import themeColors from '../styles/themes/default';

const propTypes = {
    /** Should we show the checkmark inside the circle */
    isChecked: PropTypes.bool,
    isDisabled: PropTypes.bool,
};

const defaultProps = {
    isChecked: false,
    isDisabled: false,
};

function SelectCircle(props) {
    return (
        <View style={[styles.selectCircle, styles.alignSelfCenter, props.isDisabled ? styles.disableSelectCircle : '']}>
            {props.isChecked && (
                <Icon
                    src={Expensicons.Checkmark}
                    fill={themeColors.iconSuccessFill}
                />
            )}
        </View>
    );
}

SelectCircle.propTypes = propTypes;
SelectCircle.defaultProps = defaultProps;
SelectCircle.displayName = 'SelectCircle';

export default SelectCircle;
image

3. Make another style object(I name as disabledSelectCircle) inside styles.js file

disableSelectCircle: {
        ...cursor.cursorDisabled,
},
image

What alternative solutions did you explore? (Optional)
NA

Video Attachment

disableCursor.mp4

@tjferriss tjferriss removed their assignment Sep 25, 2023
@tjferriss
Copy link
Contributor

@muttmuure it looks like MelvinBot assigned me after you. I'm going to un-assign myself.

@astrohunter62
Copy link
Contributor

Proposal

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

The cursor remains a hand icon when hovering over the checkbox in the 'Paid By' section during the Split bill process.

What is the root cause of that problem?

In the OptionRow component, there's a missing pass of the 'disabled' props to the SelectCircle component wrapped within PressableWithFeedback.

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

We can pass the 'disabled' props to PressableWithFeedback.

<PressableWithFeedback
    onPress={() => this.props.onSelectedStatePressed(this.props.option)}
    accessibilityRole={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
    accessibilityLabel={CONST.ACCESSIBILITY_ROLE.CHECKBOX}
    disabled={this.state.isDisabled}
>
    <SelectCircle isChecked={this.props.isSelected} />
</PressableWithFeedback>

Result:

28064.chrome.webm
28064.safari.webm

@melvin-bot
Copy link

melvin-bot bot commented Sep 28, 2023

@muttmuure, @aimane-chnaif Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot
Copy link

melvin-bot bot commented Sep 30, 2023

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot
Copy link

melvin-bot bot commented Oct 2, 2023

@muttmuure, @aimane-chnaif Still overdue 6 days?! Let's take care of this!

@melvin-bot
Copy link

melvin-bot bot commented Oct 4, 2023

@muttmuure, @aimane-chnaif 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@muttmuure
Copy link
Contributor

@aimane-chnaif 3 proposals to review when you have time. Thank you!

@melvin-bot melvin-bot bot removed the Overdue label Oct 4, 2023
@aimane-chnaif
Copy link
Contributor

@esh-g's proposal looks good to me.
🎀 👀 🎀 C+ reviewed

@aimane-chnaif
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: Simplify Global Create menu #25564
  • 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: https://github.com/Expensify/App/pull/25564/files#r1365623813
  • 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: N/A
  • Determine if we should create a regression test for this bug.
  • 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 regression test

@melvin-bot melvin-bot bot added the Overdue label Oct 23, 2023
@melvin-bot melvin-bot bot removed the Overdue label Oct 23, 2023
@aimane-chnaif
Copy link
Contributor

@lakchote can you please re-open the issue? I haven't received payment yet

1 similar comment
@tewodrosGirmaA
Copy link

@lakchote can you please re-open the issue? I haven't received payment yet

@dylanexpensify dylanexpensify reopened this Nov 8, 2023
@muttmuure
Copy link
Contributor

Contracts extended

@aimane-chnaif
Copy link
Contributor

@muttmuure there's contract already - #28064 (comment)

@melvin-bot melvin-bot bot added the Overdue label Nov 10, 2023
@aimane-chnaif
Copy link
Contributor

This is eligible for bonus

@melvin-bot melvin-bot bot removed the Overdue label Nov 13, 2023
@muttmuure
Copy link
Contributor

Bonus sent

@tewodrosGirmaA
Copy link

Hello @muttmuure, I have not received payment?

1 similar comment
@tewodrosGirmaA
Copy link

Hello @muttmuure, I have not received payment?

@mountiny mountiny reopened this Nov 13, 2023
@esh-g
Copy link
Contributor

esh-g commented Nov 13, 2023

@muttmuure I still haven't received the payment as well...

@muttmuure
Copy link
Contributor

Oops, sorry!

@muttmuure
Copy link
Contributor

Offers sent

@tewodrosGirmaA
Copy link

Offers sent

Thank you @muttmuure I accepted the offer

@esh-g
Copy link
Contributor

esh-g commented Nov 16, 2023

Thanks, accepted the offer @muttmuure

@esh-g
Copy link
Contributor

esh-g commented Nov 20, 2023

@muttmuure Please also make sure to check the bonus... thanks

@melvin-bot melvin-bot bot added the Overdue label Nov 22, 2023
@lakchote
Copy link
Contributor

@muttmuure were you able to check the bonus? Thanks!

@melvin-bot melvin-bot bot removed the Overdue label Nov 22, 2023
@muttmuure
Copy link
Contributor

$750 inc Bonus paid to @esh-g
@tewodrosGirmaA also paid

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