Pay Header improvements #18883
Pay Header improvements #18883
Conversation
| @@ -27,10 +27,6 @@ const propTypes = { | |||
| vertical: PropTypes.oneOf(_.values(CONST.MODAL.ANCHOR_ORIGIN_VERTICAL)), | |||
| }), | |||
|
|
|||
| /** A function with content to measure. This component will use this.props.children by default, | |||
| but in the case the children are not displayed, the measurement will not work. */ | |||
| measureContent: PropTypes.func.isRequired, | |||
There was a problem hiding this comment.
It's clear to me that this was not needed. It also said it would use this.props.children by default, but it did not. Everywhere this component was used we had measureContent be an exact or near-exact duplicate of the component's children.
Furthermore, children and measureContent were swapped, such that the children would be rendered invisibly + measured, then whatever was returned by measureContent would be rendered visibly with the adjusted position.
|
@best-lucky1030 WIP stands for "work in progress" |
|
@arosiclair 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] |
|
How was this merged without an approval |
|
It was merged into my PR, we will do reviews, tests and everything in the actual PR that is targeting main |
|
oh I see, always miss that nuance thanks |
|
🚀 Deployed to staging by https://github.com/iwiznia in version: 1.3.16-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀
|
2 similar comments
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.16-7 🚀
|
| export default function useKeyboardShortcut(shortcut, callback, config = {}) { | ||
| const {captureOnInputs = true, shouldBubble = false, priority = 0, shouldPreventDefault = true, excludedNodes = [], isActive = true} = config; | ||
|
|
||
| const subscription = useRef(null); | ||
| const subscribe = useCallback( | ||
| () => | ||
| KeyboardShortcut.subscribe( | ||
| shortcut.shortcutKey, | ||
| callback, | ||
| shortcut.descriptionKey, | ||
| shortcut.modifiers, | ||
| captureOnInputs, | ||
| shouldBubble, | ||
| priority, | ||
| shouldPreventDefault, | ||
| excludedNodes, | ||
| ), | ||
| [callback, captureOnInputs, excludedNodes, priority, shortcut.descriptionKey, shortcut.modifiers, shortcut.shortcutKey, shouldBubble, shouldPreventDefault], | ||
| ); | ||
|
|
||
| useEffect(() => { | ||
| const unsubscribe = subscription.current || (() => {}); | ||
| unsubscribe(); | ||
| subscription.current = isActive ? subscribe() : null; | ||
| return isActive ? subscription.current : () => {}; | ||
| }, [isActive, subscribe]); | ||
| } |
There was a problem hiding this comment.
I'm not sure why are we calling manually unsubscribe(); instead of letting the unsubscribe be called during cleanup. I also don't think the reference is needed or the useCallback.
I think this could have been simple to implement like this:
export default function useKeyboardShortcut(shortcut, callback, config = {}) {
const {captureOnInputs = true, shouldBubble = false, priority = 0, shouldPreventDefault = true, excludedNodes = [], isActive = true} = config;
useEffect(() => {
if (isActive) {
return KeyboardShortcut.subscribe(
shortcut.shortcutKey,
callback,
shortcut.descriptionKey,
shortcut.modifiers,
captureOnInputs,
shouldBubble,
priority,
shouldPreventDefault,
excludedNodes,
);
}
return undefined;
}, [isActive, callback, captureOnInputs, excludedNodes, priority, shortcut.descriptionKey, shortcut.modifiers, shortcut.shortcutKey, shouldBubble, shouldPreventDefault]);
}unless I'm missing something?
Also, where we use the hook here:
App/src/components/PopoverMenu/index.js
Lines 52 to 62 in 63f314c
We didn't use useCallback for keeping the same reference of the function. We re-create a new function every render which causes the useEffect inside useKeyboardShortcut to run every single time after every render.
You can see this problem if you add some console logs:
Screen.Recording.2023-05-25.at.12.56.27.PM.mov
There was a problem hiding this comment.
I spent some time trying to fix this by using useCallback and useMemo to try to keep the same references as much as possible, but I died at ReportActionCompose. The size and it being a class component doesn't help at all with it :(
There was a problem hiding this comment.
@aldo-expensify Should we try to export this to expert agency? They might have more luck with this and it will save your valuable time
There was a problem hiding this comment.
Yes, I think this can be pretty time consuming, it would be great to have their help 🙏 . Thanks!
There was a problem hiding this comment.
btw, the constant re-subscribe of the shortcut causes this bug: #18883 (comment)
There was a problem hiding this comment.
@aldo-expensify it looks like your implementation would cause a duplicate subscription to be set up every time any of the dependencies to the useEffect hook change, without cleaning up the previous subscription. That's why we're manually calling unsubscribe in the hook instead of only doing it during the cleanup.
There was a problem hiding this comment.
Seems like a potential use case for the new useEvent React hook.
There was a problem hiding this comment.
@aldo-expensify it looks like your implementation would cause a duplicate subscription to be set up every time any of the dependencies to the
useEffecthook change, without cleaning up the previous subscription. That's why we're manually callingunsubscribein the hook instead of only doing it during the cleanup.
hmm I don't think so, my implementation returns the unsubscribe function (return KeyboardShortcut.subscribe(...)). The useEffect calls the function during clean up, so it shouldn't be subscribed twice.
The useEffect does clean up before executing the callback you pass to it again.
useEffectruns a first time, we return theunsubscribecallback- Watched values change
useEffectexecutes theunsubscribecallback we returned in 1. (this is theuseEffectclean up)useEffectruns again, and we return a newunsubscribecallback- Watched values change
useEffectexecutes theunsubscribecallback we returned in 4. (this is theuseEffectclean up)useEffectruns again, and we return a newunsubscribecallback
... and so on.
There was a problem hiding this comment.
Oh, I see why I'm wrong here. The cleanup function is called every time the dependencies to the effect change. So if we need to resubscribe, the cleanup (unsubscribe) will be called first, then the effect will run and resubscribe
There was a problem hiding this comment.
That is my understanding, I did some testing adding console.logs
| useKeyboardShortcut( | ||
| CONST.KEYBOARD_SHORTCUTS.ENTER, | ||
| () => { | ||
| if (focusedIndex === -1) { | ||
| return; | ||
| } | ||
| selectItem(focusedIndex); | ||
| setFocusedIndex(-1); // Reset the focusedIndex on selecting any menu | ||
| }, | ||
| {isActive: props.isVisible}, | ||
| ); |
There was a problem hiding this comment.
I think the changes in the code that cause this event listener to constantly resubscribe (more details), make it climb to the top in the list of event handlers with the same priority. Since this event doesn't allow bubbling, it prevents any other handler. This causes this bug:
- Open the panel to invite new members to a workspace (
/workspace/<workspace id>/invite) - Verify that arrows up and down work and they change the selected row
- Select something with the mouse
- Click
Next - Go back
- BUG: The arrow keys don't work anymore
Listeners before re-subscribe:
Listeners after re-subscribe:
Context: I'm coming from debugging this other bug related to competing listeners: #18419
| */ | ||
| export default function useArrowKeyFocusManager({maxIndex, onFocusedIndexChange = () => {}, initialFocusedIndex = 0, disabledIndexes = [], shouldExcludeTextAreaNodes = true}) { | ||
| const [focusedIndex, setFocusedIndex] = useState(initialFocusedIndex); | ||
| useEffect(() => onFocusedIndexChange(focusedIndex), [focusedIndex, onFocusedIndexChange]); |
There was a problem hiding this comment.
We might have caused a bug here.
More details - #40483 (comment)
Details
Fixed Issues
$ GH_LINK
PROPOSAL: GH_LINK_ISSUE(COMMENT)
Tests
Offline tests
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