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] The cursor is displayed as pointer on the archived room banner text #13452

Closed
kavimuru opened this issue Dec 9, 2022 · 56 comments
Closed
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 Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Dec 9, 2022

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


Actions Performed:

  1. Go to any archived room
  2. Hover over the “This chat room has been archived.” text

Expected Result:

Standard cursor is displayed as the text is not pressable

Actual Result:

Pointer/hand cursor is displayed

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web

Version Number: 1.2.37-0
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Recording.1073.mp4
Screen.Recording.2022-12-08.at.9.42.08.PM.mov

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

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01776d53155c170045
  • Upwork Job ID: 1601274785687920640
  • Last Price Increase: 2022-12-12
@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 9, 2022

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

@syedsaroshfarrukhdot
Copy link
Contributor

Proposal :-

We need to pass pointerEventsNone to text

diff --git a/src/components/Banner.js b/src/components/Banner.js
index 3b5a8d66e..6e7558eb8 100644
--- a/src/components/Banner.js
+++ b/src/components/Banner.js
@@ -12,6 +12,7 @@ import * as StyleUtils from '../styles/StyleUtils';
 import getButtonState from '../libs/getButtonState';
 import Tooltip from './Tooltip';
 import withLocalize, {withLocalizePropTypes} from './withLocalize';
+import pointerEventsNone from '../styles/pointerEventsNone';
 
 const propTypes = {
     /** Text to display in the banner. */
@@ -76,7 +77,7 @@ const Banner = props => (
                     {
                         props.shouldRenderHTML
                             ? <RenderHTML html={props.text} />
-                            : <Text style={[...props.textStyles]} onPress={props.onPress}>{props.text}</Text>
+                            : <Text style={[...props.textStyles, pointerEventsNone]} onPress={props.onPress}>{props.text}</Text>
                     }

After Fix:-

Screen.Recording.2022-12-09.at.5.46.42.AM.mov

@adeel0202
Copy link
Contributor

adeel0202 commented Dec 9, 2022

I reported this bug and proposed the solution on slack. Posting it here too.

Proposal

Inside the Banner.js component, onPress is added to the Text component on line 79, which makes this text pressable. I don't think that onPress is being used or needed there, so removing that will fix this issue.

-  <Text style={[...props.textStyles]} onPress={props.onPress}>{props.text}</Text>
+  <Text style={[...props.textStyles]}>{props.text}</Text>

Result

cursor.result.mov

@syedsaroshfarrukhdot
Copy link
Contributor

syedsaroshfarrukhdot commented Dec 9, 2022

Update Proposal

As we need onPress In Some Situations We Can Take A New Props isPressable in ArchivedReportFooter to pass in Banner.js

diff --git a/src/components/ArchivedReportFooter.js b/src/components/ArchivedReportFooter.js
index c271b0f8c..d05f0441a 100644
--- a/src/components/ArchivedReportFooter.js
+++ b/src/components/ArchivedReportFooter.js
@@ -71,6 +71,7 @@ const ArchivedReportFooter = (props) => {
             })}
             shouldRenderHTML={archiveReason !== CONST.REPORT.ARCHIVE_REASON.DEFAULT}
             shouldShowIcon
+            isPressable ={archiveReason === CONST.REPORT.ARCHIVE_REASON.DEFAULT}
         />
     );
 };

diff --git a/src/components/Banner.js b/src/components/Banner.js
index 3b5a8d66e..b6393f1cc 100644
--- a/src/components/Banner.js
+++ b/src/components/Banner.js
@@ -38,6 +38,9 @@ const propTypes = {
     // eslint-disable-next-line react/forbid-prop-types
     textStyles: PropTypes.arrayOf(PropTypes.object),
 
+    /** Should this component show the pointer? */
+    isPressable: PropTypes.bool,
+
     ...withLocalizePropTypes,
 };
 
@@ -76,6 +79,7 @@ const Banner = props => (
                     {
                         props.shouldRenderHTML
                             ? <RenderHTML html={props.text} />
+                            : props.isPressable ? <Text style={[...props.textStyles]}>{props.text}</Text> 
                             : <Text style={[...props.textStyles]} onPress={props.onPress}>{props.text}</Text>
                     }
                 </View>

After Fix :-

Screen.Recording.2022-12-09.at.9.03.54.AM.mov

@adeel0202
Copy link
Contributor

Proposal 2

I don't think we need onPress in the text component as that text component only renders when shouldRenderHTML prop is false and shouldRenderHTML prop is false only when the archiveReason is default i.e. this case.

However, if we still don't want to remove onPress due to any reason, then we can either use shouldRenderHTML prop to add pressable condition or we can create a separate prop for it i.e. isTextPressable.

--- a/src/components/ArchivedReportFooter.js
+++ b/src/components/ArchivedReportFooter.js
@@ -70,6 +70,7 @@ const ArchivedReportFooter = (props) => {
                 policyName: `<strong>${ReportUtils.getPolicyName(props.report, props.policies)}</strong>`,
             })}
             shouldRenderHTML={archiveReason !== CONST.REPORT.ARCHIVE_REASON.DEFAULT}
+            isTextPressable={archiveReason !== CONST.REPORT.ARCHIVE_REASON.DEFAULT}
             shouldShowIcon
         />
     );
--- a/src/components/Banner.js
+++ b/src/components/Banner.js
@@ -26,6 +26,9 @@ const propTypes = {
     /** Should this component render the text as HTML? */
     shouldRenderHTML: PropTypes.bool,
 
+    /** Should the text component be pressable */
+    isTextPressable: PropTypes.bool,
+
     /** Callback called when the close button is pressed */
     onClose: PropTypes.func,
 
@@ -43,6 +46,7 @@ const propTypes = {
 
 const defaultProps = {
     shouldRenderHTML: false,
+    isTextPressable: true,
     shouldShowIcon: false,
     shouldShowCloseButton: false,
     onClose: () => {},
@@ -76,7 +80,7 @@ const Banner = props => (
                     {
                         props.shouldRenderHTML
                             ? <RenderHTML html={props.text} />
-                            : <Text style={[...props.textStyles]} onPress={props.onPress}>{props.text}</Text>
+                            : <Text style={[...props.textStyles]} onPress={props.isTextPressable ? props.onPress : undefined}>{props.text}</Text>
                     }
                 </View>
                 {props.shouldShowCloseButton && (

@puneetlath puneetlath added the External Added to denote the issue can be worked on by a contributor label Dec 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 9, 2022

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

@melvin-bot melvin-bot bot changed the title The cursor is displayed as pointer on the archived room banner text [$1000] The cursor is displayed as pointer on the archived room banner text Dec 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 9, 2022

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

@melvin-bot
Copy link

melvin-bot bot commented Dec 9, 2022

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 9, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 9, 2022

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

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Dec 9, 2022

Proposal
We can simply remove default prop (passing function) which is creating issue. React Native Text and Pressable both onPress are allowed to be undefined.
/src/components/Banner.js
Screenshot 2022-12-09 at 11 57 51 PM

@jayeshmangwani
Copy link
Contributor

Proposal

we should pass the pointernone style from the footer component

<Banner
text={props.translate(`reportArchiveReasons.${archiveReason}`, {
displayName: `<strong>${displayName}</strong>`,
oldDisplayName: `<strong>${oldDisplayName}</strong>`,
policyName: `<strong>${ReportUtils.getPolicyName(props.report, props.policies)}</strong>`,
})}
shouldRenderHTML={archiveReason !== CONST.REPORT.ARCHIVE_REASON.DEFAULT}
shouldShowIcon
/>

+             textStyles={[styles.pointerEventsNone]}

after applying the fix

13452-fixed.mov

@priyeshshah11
Copy link
Contributor

Proposal

Problem

The onPress && onClose props have an empty anonymous function as its default value, thus the text shows the pointer on the hover event.

Solution 1

Set both onPress && onClose props' default values to undefined in Banner.js

--- a/src/components/Banner.js
+++ b/src/components/Banner.js
@@ -45,53 +45,56 @@ const defaultProps = {
     shouldRenderHTML: false,
     shouldShowIcon: false,
     shouldShowCloseButton: false,
-    onClose: () => {},
-    onPress: () => {},
+    onClose: undefined,
+    onPress: undefined,
     containerStyles: [],
     textStyles: [],
 };

Solution 2: Addition to Solution 1

I think we should not highlight the banner if it is not clickable, as highlighting (i.e. changing the background colour on hover) indicates to the user as if it is clickable. This might be a question for the design time to confirm.

 const Banner = props => (
     <Hoverable>
-        {isHovered => (
-            <View style={[
+        {(isHovered) => {
+            const isClickable = props.onClose || props.onPress;
+            return (
+                <View style={[

-                isHovered ? styles.activeComponentBG : styles.hoveredComponentBG,
+                    isClickable && isHovered ? styles.activeComponentBG : styles.hoveredComponentBG,

Optional: as this might be a question for the design time to whether highlight the icon or not.
-                            <Icon
-                                src={Expensicons.Exclamation}
-                                fill={StyleUtils.getIconFillColor(getButtonState(isHovered))}
+                                <Icon
+                                    src={Expensicons.Exclamation}
+                                    fill={StyleUtils.getIconFillColor(getButtonState(isClickable && isHovered))}
Screen.Recording.2022-12-10.at.2.46.51.PM.mov

@Santhosh-Sellavel @puneetlath

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Dec 10, 2022

Proposal 2
If we want to remove hover effect also we can add enabled prop in Hoverable component. With this it will also remove unnecessary listener and usable on another area also where we may face same issue. So combine with my above solution we can do

diff --git a/src/components/Banner.js b/src/components/Banner.js
index 3b5a8d66e3..bfb066fa8c 100644
--- a/src/components/Banner.js
+++ b/src/components/Banner.js
@@ -46,13 +46,13 @@ const defaultProps = {
     shouldShowIcon: false,
     shouldShowCloseButton: false,
     onClose: () => {},
-    onPress: () => {},
+    onPress: undefined,
     containerStyles: [],
     textStyles: [],
 };
 
 const Banner = props => (
-    <Hoverable>
+    <Hoverable enabled={!!props.onPress}>
         {isHovered => (
             <View style={[
                 styles.flexRow,
diff --git a/src/components/Hoverable/hoverablePropTypes.js b/src/components/Hoverable/hoverablePropTypes.js
index 07b8a8741e..2978a6c7f1 100644
--- a/src/components/Hoverable/hoverablePropTypes.js
+++ b/src/components/Hoverable/hoverablePropTypes.js
@@ -22,10 +22,14 @@ const propTypes = {
 
     // If the mouse clicks outside, should we dismiss hover?
     resetsOnClickOutside: PropTypes.bool,
+
+    /** Should hover able enabled */
+    enabled: PropTypes.bool,
 };
 
 const defaultProps = {
     absolute: false,
+    enabled: true,
     containerStyles: [],
     onHoverIn: () => {},
     onHoverOut: () => {},
diff --git a/src/components/Hoverable/index.js b/src/components/Hoverable/index.js
index f06ed56027..6726834eb7 100644
--- a/src/components/Hoverable/index.js
+++ b/src/components/Hoverable/index.js
@@ -21,6 +21,10 @@ class Hoverable extends Component {
     }
 
     componentDidMount() {
+        if (!this.props.enabled) {
+            return;
+        }
+
         document.addEventListener('mousedown', this.resetHoverStateOnOutsideClick);
 
         // we like to Block the hover on touch devices but we keep it for Hybrid devices so
@@ -82,6 +86,12 @@ class Hoverable extends Component {
     }
 
     render() {
+        if (!this.props.enabled) {
+            return _.isFunction(this.props.children)
+                ? this.props.children(this.state.isHovered)
+                : this.props.children;
+        }
+
         if (this.props.absolute && React.isValidElement(this.props.children)) {
             return React.cloneElement(React.Children.only(this.props.children), {
                 ref: (el) => {

@Santhosh-Sellavel
Copy link
Collaborator

Thanks, everyone!

I straight away gave 👎 to proposals that could break something, and other proposals are being considered. But before moving forward let's confirm what's the expectation here.

@Expensify/design

Screen.Recording.2022-12-11.at.10.57.19.PM.mov

On archived workspace chats, we show a banner and related messages. Based on the implementation the banner click doesn't have any actions. Showing clickable pointer is an issue will be removed, additionally, should we keep the hover effect or not?

cc: @puneetlath

@shawnborton
Copy link
Contributor

Yeah, I'm not entirely sure why this banner has a cursor or why it has a hover state. I would think it would just show as a banner with no interactive elements unless we put some kind of explicit link in there (almost like a growl might have).

@shawnborton
Copy link
Contributor

Also, I wonder if we want to just make this one internal since @srikarparsi is already working on an issue related to this component: #13373

@jatinsonijs
Copy link
Contributor

jatinsonijs commented Dec 12, 2022

Banner is using on 2 places one as above issue another as below SS. Here on press text its navigate to another screen. I just displayed it for testing by changing condition.

Screenshot 2022-12-12 at 9 43 01 AM

@srikarparsi
Copy link
Contributor

yeah I can take this one but what do you think @puneetlath since a lot of people have been working on this issue? Also, @shawnborton, do we want a hover state? I don't really think we should have one since it indicates that it's clickable but just wanted to check?

@shawnborton
Copy link
Contributor

shawnborton commented Dec 12, 2022 via email

@0xmiros
Copy link
Contributor

0xmiros commented Dec 12, 2022

Proposal

Introduce new prop disabled in Banner component

const propTypes = {
    ...

    // eslint-disable-next-line react/forbid-prop-types
    textStyles: PropTypes.arrayOf(PropTypes.object),

+   disabled: PropTypes.bool,

    ...withLocalizePropTypes,
};
const defaultProps = {
    shouldRenderHTML: false,
    shouldShowIcon: false,
    shouldShowCloseButton: false,
    onClose: () => {},
    onPress: () => {},
    containerStyles: [],
    textStyles: [],
+   disabled: false,
};
            <View style={[
                styles.flexRow,
                styles.alignItemsCenter,
                styles.p5,
                styles.borderRadiusNormal,
-               isHovered ? styles.activeComponentBG : styles.hoveredComponentBG,
+               isHovered && !props.disabled ? styles.activeComponentBG : styles.hoveredComponentBG,
                styles.breakAll,
                ...props.containerStyles,
            ]}
            >
                <View style={[styles.flexRow, styles.flexGrow1, styles.mw100, styles.alignItemsCenter]}>
                    {props.shouldShowIcon && (
                        <View style={[styles.mr3]}>
                            <Icon
                                src={Expensicons.Exclamation}
-                               fill={StyleUtils.getIconFillColor(getButtonState(isHovered))}
+                               fill={StyleUtils.getIconFillColor(getButtonState(isHovered && !props.disabled))}
                            />
                        </View>
                    )}
                    {
                        props.shouldRenderHTML
                            ? <RenderHTML html={props.text} />
-                           : <Text style={[...props.textStyles]} onPress={props.onPress}>{props.text}</Text>
+                           : <Text style={[...props.textStyles]} onPress={props.disabled ? null : props.onPress}>{props.text}</Text>
                    }
                </View>

In ArchivedReportFooter:

        <Banner
            text={props.translate(`reportArchiveReasons.${archiveReason}`, {
                displayName: `<strong>${displayName}</strong>`,
                oldDisplayName: `<strong>${oldDisplayName}</strong>`,
                policyName: `<strong>${ReportUtils.getPolicyName(props.report, props.policies)}</strong>`,
            })}
            shouldRenderHTML={archiveReason !== CONST.REPORT.ARCHIVE_REASON.DEFAULT}
            shouldShowIcon
+           disabled
        />

@adeel0202
Copy link
Contributor

Updated Proposal

To disable the hover effect, we can use the same isTextPressable prop that I proposed in this proposal and disable hover in this case.

Only these new changes will be required to achieve this.

- isHovered ? styles.activeComponentBG : styles.hoveredComponentBG,
+ isHovered && props.isTextPressable ? styles.activeComponentBG : styles.hoveredComponentBG,

- fill={StyleUtils.getIconFillColor(getButtonState(isHovered))}
+ fill={StyleUtils.getIconFillColor(getButtonState(isHovered && props.isTextPressable))}

Result

Screen.Recording.2022-12-12.at.3.24.42.PM.mov

@Santhosh-Sellavel
Copy link
Collaborator

@srikarparsi Are you gonna handle this internally? Let me know so that we can decide on making a hiring decision here.

cc: @puneetlath

@srikarparsi
Copy link
Contributor

Hey @Santhosh-Sellavel, could you please review the current set of proposals and if something looks good, we'll go with it.
But if none of them work, I'll take this internally for urgency.

@jatinsonijs
Copy link
Contributor

No problem @Santhosh-Sellavel, Thank you for your suggestions. I will definitely consider it in my next proposal.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 13, 2022
@melvin-bot
Copy link

melvin-bot bot commented Dec 13, 2022

📣 @priyeshshah11 You have been assigned to this job by @puneetlath!
Please apply to this job in Upwork 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 📖

@priyeshshah11
Copy link
Contributor

Thanks! @puneetlath @Santhosh-Sellavel
I have applied to the upwork job and aim to get the PR up by EOD today/tomorrow. Just waiting to perform thorough testing after the login issue is fixed.

@puneetlath
Copy link
Contributor

Great thanks! @Santhosh-Sellavel will you be able to review tomorrow?

Also @adeel0202 can you apply to the job for the reporting bonus?

@Santhosh-Sellavel
Copy link
Collaborator

@puneetlath Will review it shortly!

@adeel0202
Copy link
Contributor

@puneetlath I have applied for the reporting bonus.

@melvin-bot
Copy link

melvin-bot bot commented Dec 22, 2022

@puneetlath, @priyeshshah11, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

@JmillsExpensify
Copy link

@puneetlath heads up, this PR already hit production. Looks like the GH automation didn't trigger for the regression period, though it started yesterday.

@melvin-bot
Copy link

melvin-bot bot commented Dec 26, 2022

@puneetlath, @priyeshshah11, @Santhosh-Sellavel Still overdue 6 days?! Let's take care of this!

@melvin-bot
Copy link

melvin-bot bot commented Dec 28, 2022

@puneetlath, @priyeshshah11, @Santhosh-Sellavel Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@JmillsExpensify
Copy link

@puneetlath You want me to jump in and help move this one forward?

@priyeshshah11
Copy link
Contributor

@JmillsExpensify @puneetlath This PR was merged within 3 days and was released to production a week ago. I believe it has passed the 7 days so just waiting for the payment to be finalised

@priyeshshah11
Copy link
Contributor

@JmillsExpensify @puneetlath This PR was merged within 3 days and was released to production a week ago. I believe it has passed the 7 days so just waiting for the payment to be finalised

@JmillsExpensify @puneetlath any updates on this please?

@JmillsExpensify JmillsExpensify self-assigned this Jan 2, 2023
@JmillsExpensify
Copy link

Just assigned myself to help out. Based on the linked PR, this PR is eligible for a 50% bonus. That means that the following payouts are due:

@JmillsExpensify
Copy link

Based on the above, I've just paid out @priyeshshah11 and @Santhosh-Sellavel. @adeel0202 Contract for reporting has been shared.

@JmillsExpensify
Copy link

JmillsExpensify commented Jan 2, 2023

Update: All contributors are now paid out! Sorry for the delay.

@JmillsExpensify
Copy link

@puneetlath I don't see a BZ checklist for this one. Mind chiming in on the regression/convo parts of that list?

@adeel0202
Copy link
Contributor

Thanks, @JmillsExpensify. Kindly end the contract too.

@priyeshshah11
Copy link
Contributor

Update: All contributors are now paid out! Sorry for the delay.

Thanks! 😄 @JmillsExpensify

@puneetlath
Copy link
Contributor

puneetlath commented Jan 5, 2023

@JmillsExpensify
Copy link

Are we good to close this one out?

@puneetlath
Copy link
Contributor

Yep!

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 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests