-
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
16265 - Refactor ReportActionItemMessageEdit to functional component #20296
16265 - Refactor ReportActionItemMessageEdit to functional component #20296
Conversation
@roryabraham 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] |
I will review this @roryabraham cc @yuwenmemon |
@roryabraham Please re-review once, I have applied changes. |
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.
Looks better to me, but let's get the C+ review here for testing.
const emojiButtonID = 'emojiButton'; | ||
const messageEditInput = 'messageEditInput'; | ||
|
||
function ReportActionItemMessageEdit({action, forwardedRef, draftMessage, index, isKeyboardShown, isSmallScreenWidth, preferredSkinTone, reportID, shouldDisableEmojiPicker, translate}) { |
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.
This is against our style guidance. Use props
as the function parameter and do not destruct the props, just use props.action
, props.forwardedRef
, etc.
setDraft((prevDraft) => { | ||
setSelection({ | ||
start: prevDraft.length, | ||
end: prevDraft.length, | ||
}); | ||
return prevDraft; | ||
}); |
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.
Do we have to call setDraft
to access the previous draft? Is there any other options here?
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.
We are setting old value to setDraft method. it will not cause re-render.
though we can directly access draft
value and use it. as we just have to do it at mount only:
setSelection({
start: draft.length,
end: draft.length,
});
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.
@s77rt need your input on this.
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.
I don't think using draft will work, we will have to add it as a dependency.
Maybe we should just keep it as it's now?
@yuwenmemon Any thoughts on this?
useEffect( | ||
() => () => { | ||
// Skip if this is not the focused message so the other edit composer stays focused. | ||
if (!isFocused) { | ||
return; | ||
} | ||
|
||
/** | ||
* Update Selection on change cursor position. | ||
* | ||
* @param {Event} e | ||
*/ | ||
onSelectionChange(e) { | ||
this.setState({selection: e.nativeEvent.selection}); | ||
} | ||
// Show the main composer when the focused message is deleted from another client | ||
// to prevent the main composer stays hidden until we swtich to another chat. | ||
ComposerActions.setShouldShowComposeInput(true); | ||
}, | ||
// eslint-disable-next-line react-hooks/exhaustive-deps -- for willUnmount lifecycle | ||
[], | ||
); |
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.
Can we remove this useEffect
and just use the previous one?
One useEffect
can handle both componentDidMount
and componentWillUnmount
phases.
updateDraft(draft) { | ||
const {text: newDraft = '', emojis = []} = EmojiUtils.replaceEmojis(draft, this.props.isSmallScreenWidth, this.props.preferredSkinTone); | ||
const updateDraft = useCallback( | ||
(newDraftValue) => { |
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.
NAB can you please change the variable name to newDraftInput.
if (newDraftValue !== newDraft) { | ||
setSelection((prevSelection) => { | ||
const remainder = draft.slice(prevSelection.end).length; | ||
return { | ||
start: newDraft.length - remainder, | ||
end: newDraft.length - remainder, | ||
}; | ||
}); | ||
} | ||
setDraft(newDraft); |
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.
Can we rewrite this as:
setDraft((prevDraft) => {
if (newDraftValue !== newDraft) {
setSelection((prevSelection) => {
const remainder = prevDraft.slice(prevSelection.end).length;
return {
start: newDraft.length - remainder,
end: newDraft.length - remainder,
};
});
}
return newDraft;
});
const addEmojiToTextBox = useCallback( | ||
(emoji) => { | ||
setSelection((prevSelection) => ({ | ||
start: prevSelection.start + emoji.length, | ||
end: prevSelection.start + emoji.length, | ||
})); | ||
updateDraft(ComposerUtils.insertText(draft, selection, emoji)); | ||
}, | ||
[draft, selection, updateDraft], | ||
); |
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.
In which cases the component would re-render and have same draft and selection? I don't think this is common so the useCallback
may be a bad choice here since we will mostly end up redefining the function.
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.
So should i remove useCallback here?
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.
Yes
@kushu7 Can you please push commits for requested changes that we agreed on |
Just pushed, Please review it. |
return () => { | ||
// Skip if this is not the focused message so the other edit composer stays focused. | ||
if (!isFocused) { | ||
return; | ||
} | ||
|
||
/** | ||
* Update Selection on change cursor position. | ||
* | ||
* @param {Event} e | ||
*/ | ||
onSelectionChange(e) { | ||
this.setState({selection: e.nativeEvent.selection}); | ||
} | ||
// Show the main composer when the focused message is deleted from another client | ||
// to prevent the main composer stays hidden until we swtich to another chat. | ||
ComposerActions.setShouldShowComposeInput(true); | ||
}; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps -- we need it to call it once on mount | ||
}, []); |
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.
I'm not sure about the cleanup function here. Won't isFocused
always be false
(as that's it's initial state).
The cleanup function is evaluated now but executed later or am I missing something?
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.
It will get latest value.
Deps array particularly we use when we have to do something when deps are changing.
In cleanup. Function will get latest value of isFocused not initial value
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.
@s77rt you are right, we should replace it by useRef
. as ref will always return latest value.
const textInputRef = useRef(null);
const isFocusedRef = useRef(false);
useEffect(() => {
// For mobile Safari, updating the selection prop on an unfocused input will cause it to automatically gain focus
// and subsequent programmatic focus shifts (e.g., modal focus trap) to show the blue frame (:focus-visible style),
// so we need to ensure that it is only updated after focus.
setDraft((prevDraft) => {
setSelection({
start: prevDraft.length,
end: prevDraft.length,
});
return prevDraft;
});
return () => {
// Skip if this is not the focused message so the other edit composer stays focused.
if (!isFocusedRef.current) {
return;
}
// Show the main composer when the focused message is deleted from another client
// to prevent the main composer stays hidden until we swtich to another chat.
ComposerActions.setShouldShowComposeInput(true);
};
// eslint-disable-next-line react-hooks/exhaustive-deps -- we need it to call it once on mount
}, []);
and in composer props
onFocus={() => {
setIsFocused(true);
isFocusedRef.current = true;
ReportScrollManager.scrollToIndex({animated: true, index: props.index}, true);
ComposerActions.setShouldShowComposeInput(false);
}}
onBlur={(event) => {
setIsFocused(false);
isFocusedRef.current = false;
What do you say?
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.
Example i created: https://codesandbox.io/s/cranky-bush-8eqcew
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.
This works but it's not ideal to have two variables that hold the same value. Let's fix it in another way
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.
Unfortunately this is not working as expected. The isFocused
method seems to return false
where the isFocused
state is still set to true
. This will reintroduce this bug #16623.
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.
@s77rt are you sure? I tested by console.log
In another useEffect method with isFocused as deps.
Both were showing same value. And textInput.isFocused() is already used in some components.
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.
Kooha-2023-06-08-12-23-47.mp4
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.
I got it now as unmount state ( cleanup fn) we process using previous state. but isFocused()
return us current state of component.
I think we proceed with ref variable for keeping focus state. as its synced with isFocused
state.
const isFocusedRef = useRef(false);
useEffect(() => {
isFocusedRef.current = isFocused
}, [isFocused])
@s77rt what do you think?
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.
I don't think this is the ideal approach, there could be an easy solution that we are missing. Can you please ask on Slack, given that there is many ongoing functional refactor maybe someone encountered a similar case.
@s77rt Please have a look, I have pushed the changes. |
// to prevent the main composer stays hidden until we swtich to another chat. | ||
ComposerActions.setShouldShowComposeInput(true); | ||
}; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps -- we need it to call it once on mount |
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.
// eslint-disable-next-line react-hooks/exhaustive-deps -- we need it to call it once on mount |
Not needed, we are using a ref now and it always have the same value so no need for it to be a dependency (and no error will be thrown).
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.
Forgot about it. I have removed it now.
Reviewer Checklist
Screenshots/VideosWebweb.mp4Mobile Web - Chromemweb-chrome.mp4Mobile Web - Safarimweb-safari.mp4Desktopdesktop.mp4iOSios.mp4Androidandroid.mp4 |
@@ -1,6 +1,7 @@ | |||
/* eslint-disable rulesdir/onyx-props-must-have-default */ | |||
import lodashGet from 'lodash/get'; | |||
import React from 'react'; | |||
import React, {useState, useRef, useMemo, useEffect, useCallback} from 'react'; | |||
// eslint-disable-next-line no-restricted-imports |
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.
There are no restricted imports on the next line AFAICT, so let's remove this:
// eslint-disable-next-line no-restricted-imports |
} | ||
|
||
ReportActionItemMessageEdit.propTypes = propTypes; | ||
ReportActionItemMessageEdit.defaultProps = defaultProps; | ||
ReportActionItemMessageEdit.displayName = 'ReportActionItemMessageEdit'; | ||
export default compose( | ||
withLocalize, |
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.
It was reverted for an unrelated reason, but let's re-introduce the useLocalize
hook from #20708 and use it here instead of withLocalize
.
} | ||
|
||
ReportActionItemMessageEdit.propTypes = propTypes; | ||
ReportActionItemMessageEdit.defaultProps = defaultProps; | ||
ReportActionItemMessageEdit.displayName = 'ReportActionItemMessageEdit'; | ||
export default compose( | ||
withLocalize, | ||
withWindowDimensions, |
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.
Let's also use useWindowDimensions
hook from src/hooks
instead of withWindowDimensions
. Also don't forget to remove the unused props from these HOCs when you remove them
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.
Lastly, let's also add a very simple useKeyboardState
hook in src/hooks
:
export default function useKeyboardState() {
const [isKeyboardShown, setIsKeyboardShown] = useState(false);
useEffect(() => {
const keyboardDidShowListener = Keyboard.addListener('keyboardDidShow', () => setIsKeyboardShown(true));
return keyboardDidShowListener.remove;
}, []);
useEffect(() => {
const keyboardDidHideListener = Keyboard.addListener('keyboardDidHide', () => setIsKeyboardShown(false));
return keyboardDidHideListener.remove;
}, []);
return {
isKeyboardShown,
};
}
Then use it like this:
const {isKeyboardShown} = useKeyboardState();
And then remove the withKeyboardState
HOC and props.
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.
We should get used to using hooks more as we migrate to a functional react style because they're often simpler to read, simpler to use, simpler to type once we start migrating everything to TypeScript, and don't bloat the component tree as much as HOCs.
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.
Actually, I see we have a context for withKeyboardState
that's preventing us from setting up duplicate listeners. So the useKeyboardState
hook becomes even simpler. Just:
-
Export
KeyboardStateContext
fromwithKeyboardState
-
Implement
useKeyboardState
like so:import {useContext} from 'react'; import {KeyboardStateContext} from '../components/withKeyboardState'; export default function useKeyboardState() { return useContext(KeyboardStateContext); }
Why Lint getting failed. weird 👀 |
@kushu7 it looks like you need to:
|
…n-item-message-edit-to-functional
Thanks, I was behind. |
// to prevent the main composer stays hidden until we swtich to another chat. | ||
ComposerActions.setShouldShowComposeInput(true); | ||
}; | ||
// eslint-disable-next-line react-hooks/exhaustive-deps |
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.
I'm unsure whether the cleanup function will behave as you expect without wasPreviouslyFocused
being a dependency 🤔
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.
I think we're okay here, but honestly not 100% sure.
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.
I think I'm just thrown off because eslint knows that if you do this:
const myRef = useRef(1);
useEffect(() => {
console.log(`Do something with ${myRef.current}`);
}, []);
Then you don't have to pass myRef
or myRef.current
as a dependency to the useEffect
. That's exactly what we're doing here, but eslint is thrown off because it doesn't know what usePrevious
is doing and doesn't know it's just returning aRef.current
under the hood.
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.
@roryabraham I tested through. I can conclude. It will not work. it holds previous value, but require wasPreviouslyFocused
to add in deps array.
I tested using that focus bug, its failing.
My thought was also same as yours. as we were moving only logic to usePreviousHook
.
usePreviousHook is hook good when we require to check old value. but its require to add it in deps array
. but in our case we can't use it as we are performing componentWillUnmount
action.
Code i used to test current state at cleanup function.
const wasPreviouslyFocused = usePrevious(isFocused);
const isFocusedRef = useRef(false);
useEffect(() => {
// required for keeping last state of isFocused variable
isFocusedRef.current = isFocused;
console.log(wasPreviouslyFocused,'<-SYNC-CHECK->',isFocused,'----',isFocusedRef.current)
}, [isFocused]);
useEffect(()=>{
return ()=>{
console.log('cleanup with dep',wasPreviouslyFocused, '-it should be isFocusedRef.current',isFocusedRef.current)
}
},[wasPreviouslyFocused])
useEffect(()=>{
return ()=>{
console.log('cleanup without dep value',wasPreviouslyFocused,'-it should be isFocusedRef.current',isFocusedRef.current)
}
},[])
useEffect(() => {
// For mobile Safari, updating the selection prop on an unfocused input will cause it to automatically gain focus
// and subsequent programmatic focus shifts (e.g., modal focus trap) to show the blue frame (:focus-visible style),
// so we need to ensure that it is only updated after focus.
setDraft((prevDraft) => {
setSelection({
start: prevDraft.length,
end: prevDraft.length,
});
return prevDraft;
});
return () => {
// Skip if this is not the focused message so the other edit composer stays focused
console.log(wasPreviouslyFocused,'<-SYNC-CHECK CLEANUP VALUE->',isFocused,'---',isFocusedRef.current)
if (!wasPreviouslyFocused) {
return;
}
// Show the main composer when the focused message is deleted from another client
// to prevent the main composer stays hidden until we swtich to another chat.
ComposerActions.setShouldShowComposeInput(true);
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Testing with usePrevious
Screen.Recording.2023-06-14.at.11.01.50.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 your thorough re-testing, it's a bummer that usePrevious
has this limitation. I imagine it would be a useful hook to build into the core React 🤔
@s77rt As this migration got many changes after last testing, I request to Re-test also at your end also. I have tested through it. |
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.
Retested. Working well from my end.
cc @yuwenmemon
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.
All yours @roryabraham
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 all those changes @kushu7 and your thorough testing. LGTM 👍🏼
✋ 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/roryabraham in version: 1.3.28-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀
|
Details
Fixed Issues
$ #16265
PROPOSAL: #16265 (comment)
Tests
Offline tests
Same as Tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
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)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
web1.mov
Mobile Web - Chrome
mweb.chrome.mp4
Mobile Web - Safari
msafari.mov
Desktop
Screen.Recording.2023-06-06.at.11.28.40.PM.mov
iOS
ios.mov
Android
android.mp4