fix send money - green payment button is visible bug #20655
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
dostongulmatov
force-pushed
the
fix/#20262-send-money-popover-position
branch
from
e1244b1
to
73858a1
Compare
|
I have read the CLA Document and I hereby sign the CLA |
|
@Beamanator Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| onPress={() => { | ||
| if (popoverAnchorPosition === null) { | ||
| measurePopoverPosition(); | ||
| } | ||
| setIsMenuVisible(true); | ||
| }} |
There was a problem hiding this comment.
Rewrite this as:
- Measure popover position
- Set popover position (inside measure callback)
- Set menu visible (also inside measure callback)
There was a problem hiding this comment.
@s77rt would be great if you give me more detailed description about changes you mentioned here, thank you
There was a problem hiding this comment.
I think you are asking me to make this callback function
const measure = () => {
if (popoverAnchorPosition === null) {
measurePopoverPosition();
}
setIsMenuVisible(true);
}
and call it in onPress of caretButton, is it correct?
There was a problem hiding this comment.
I meant something like this:
caretButton.current.measureInWindow((x, y, w, h) => {
setPopoverAnchorPosition({
horizontal: x + w,
vertical: y + h,
});
setIsMenuVisible(true);
});There was a problem hiding this comment.
okay thank you I just did it like you mensioned here and pushed my changes, please check it and let me know if we can make anything better
| measurePopoverPosition(); | ||
| } | ||
|
|
||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Why we are disabling this rule?
There was a problem hiding this comment.
@s77rt because it asks to add popoverAnchorPosition as a dependency which I think we should not add here.
| if (popoverAnchorPosition !== null) { | ||
| measurePopoverPosition(); | ||
| } |
There was a problem hiding this comment.
Remove the null check. Also in the useEffect we should only measure the popover position, currently this function do both measure the position and show the menu.
There was a problem hiding this comment.
actually I returned null check here again because if we do not do it the popoverAnchorPosition is breaking again on the first render
There was a problem hiding this comment.
I'm not sure how would this break, you need to fix the other concern as well to get this working correctly
Also in the useEffect we should only measure the popover position, currently this function do both measure the position and show the menu.
There was a problem hiding this comment.
@s77rt yes, you are actually right, so I am adding an argument called showMenu to the measurePopoverPositionAndShowMenu function and set it to true when calling from onPress of caretButton so that it will call setIsMenuVisible(true) only when we click the dropdownButton while inside useEffect it will not call, please check it let me know if we can make anything else better here
There was a problem hiding this comment.
and also es-lint disabling rule is removed as well as we do not have popoverAnchorPosition inside useEffect anymore.
| const measurePopoverPosition = () => { | ||
| caretButton.current.measureInWindow((x, y, w, h) => { | ||
| setPopoverAnchorPosition({ | ||
| horizontal: x + w, | ||
| vertical: y + h, | ||
| }); | ||
| setIsMenuVisible(true); | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Rename this function to something more fitting to its functionality e.g. measurePopoverPositionAndShowMenu or anything that may be better.
There was a problem hiding this comment.
@s77rt changed function name into measurePopoverPositionAndShowMenu
| @@ -55,25 +55,23 @@ const ButtonWithDropdownMenu = (props) => { | |||
| const {windowWidth, windowHeight} = useWindowDimensions(); | |||
| const caretButton = useRef(null); | |||
|
|
|||
| const measurePopoverPositionAndShowMenu = () => { | |||
| const measurePopoverPositionAndShowMenu = (showMenu) => { | |||
There was a problem hiding this comment.
make the default showMenu value true to be consist with the function name.
| return; | ||
| } | ||
|
|
||
| const measurePopoverPositionAndShowMenu = (showMenu = true) => { |
There was a problem hiding this comment.
Add jsdoc for the showMenu argument
There was a problem hiding this comment.
@s77rt added
Co-authored-by: Abdelhafidh Belalia <[email protected]>
| }); | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Sorry for the back and forth. I believe that code readability is much superior than performance. The current code is hard to read. I think the following version is the most balance in terms of performance and readability:
useEffect(() => {
if (!caretButton.current) {
return;
}
if (!isMenuVisible) {
return;
}
caretButton.current.measureInWindow((x, y, w, h) => {
setPopoverAnchorPosition({
horizontal: x + w,
vertical: y + h,
});
});
}, [windowWidth, windowHeight, isMenuVisible]);This will not cause flicker at the first render because the popover position won't be set so it's okay to set isMenuVisible to true (the menu won't be visible until the position is calculated).
Every other change should be reverted, we just need to update the useEffect.
There was a problem hiding this comment.
@s77rt yes I also think you are right great solution just implemented it and committed changes as you asked please check it and let me know if have to change smth
|
@dostongulmatov remember to assign yourself to the PR when you create one. Our reviewer auto-assignment doesn't work correctly if you don't do that |
Reviewer Checklist
Screenshots/VideosWeb
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
|
|
@dostongulmatov Please run prettier |
|
@s77rt |
|
@dostongulmatov update the PR description we are not adding the measure function |
|
@s77rt sure I will update it |
|
@s77rt updated the PR description |
|
✋ 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 https://github.com/arosiclair in version: 1.3.29-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.29-11 🚀
|
Details
This PR adds isMenuVisible as a dependancy to the useEffect in ButtonWithDropdownMenu component and it will call the measureInWindow only when the isMenuVisible true and when one of the dependencies of useEffect is changed.
Fixed Issues
$ #20262
$ #20262 (comment)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android