-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
IOU Contact Preview clickable cursor issue fixed #8760
IOU Contact Preview clickable cursor issue fixed #8760
Conversation
Pulled changes from Main. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and tests well 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @Santhosh-Sellavel, I was updating your QA steps to make them more thorough, then I found one edge case that isn't quite right:
- Open any group chat not including concierge or chronos.
- Press on the ➕ icon in the message composer.
- Enter an amount and press
Next
. - Hover over your own information. The cursor should appear as disabled (currently is a pointer, even though you can't deselect yourself)💥
That seems a new case to me, but I am sure it's well within the scope of this issue. I'll try to fix it by Monday or let's open it to contributors thanks! |
Can we add App/src/components/IOUConfirmationList.js Lines 223 to 228 in 3cdd9a2
And use it here App/src/components/OptionsList/BaseOptionsList.js Lines 113 to 115 in 3cdd9a2
as isDisabled={this.props.isDisabled || section.isDisabled} |
@roryabraham Here is my solution for the edge case #8760 (comment). Let me know your comments. |
bump @roryabraham |
Pushed the changes proposed here #8760 (comment) Screen.Recording.2022-05-16.at.12.21.06.AM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making that adjustment, @Santhosh-Sellavel
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @roryabraham in version: 1.1.63-0 🚀
|
🚀 Deployed to staging by @roryabraham in version: 1.1.63-0 🚀
|
🚀 Deployed to production by @AndrewGable in version: 1.1.63-2 🚀
|
Details
This PR is more specific, I tested everything works well on all platforms.
Fixed Issues
$ #8034
Tests
- hover on the participant and verify the
selection
pointer is shown- check/uncheck participant's selection
- hover on the participant and verify the
disabled cursor
pointer is shown- not able to deselect participants
- hover on the participant and verify the
disabled cursor
pointer is shownPR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)QA Steps
Next
Split Bill
.Next
.Next
.Next
Next
Request Money
Next
Next
Send money
Next
Next
@Expensify/applauseleads – please update the existing TR regression tests that cover
Split bill
,Request money
, andSend money
to include the steps here. i.e:"Hover over yourself, verify that the cursor is disabled. Verify that you can't deselect yourself."
"Hover over the other participants. If you are requesting money from or sending money to a single user, they should be disabled on hover/unselectable. If you are splitting a bill, they should be disabled/unselectable only if you initiated the split bill from an existing group chat."
Verify that no errors appear in the JS console
Screenshots
Added attachments for only desktop & web.
Desktop
Desktop_RequestSendFromFAB.mov
Desktop_RequestSendFromChat.mov
Desktop_FromFAB.mov
Desktop_FromGroup.mov
Web
Web_fromFAB.mov
Web_fromChat.mov
Web_Split_Both.mov