Skip to content

ComposeMenu [nfc]: Convert to functional component.#4796

Open
WesleyAC wants to merge 1 commit intozulip:mainfrom
WesleyAC:composemenu-functional-component
Open

ComposeMenu [nfc]: Convert to functional component.#4796
WesleyAC wants to merge 1 commit intozulip:mainfrom
WesleyAC:composemenu-functional-component

Conversation

@WesleyAC
Copy link
Copy Markdown
Contributor

This was step one in a UX change I was planning to make. I got sidetracked due to problems with the library I was planning to use (callstack/react-native-paper#2769), but I figure I'd push this refactor commit anyways.

@WesleyAC WesleyAC requested review from chrisbobbe and gnprice June 11, 2021 21:31
Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! See one small comment below.

const { destinationNarrow } = props;
const dispatch = useDispatch();

const uploadFileCallback = useCallback(
Copy link
Copy Markdown
Contributor

@chrisbobbe chrisbobbe Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This useCallback isn't doing much; uploadFileCallback's callers are functions that get recreated each time the ComposeMenu renders (they aren't using useCallback), and those functions are being passed to various children.

@gnprice
Copy link
Copy Markdown
Member

gnprice commented Jul 21, 2021

Thanks for this refactor! I agree with Chris's small comment, and everything else looks good.

Other than cutting that useCallback which isn't doing anything useful, the other direction to go would be to apply useCallback to all the named functions there, so that the callbacks passed as props to child components would be made stable. That's potentially an optimization. But we had a chat discussion a couple of weeks ago about when to use useCallback:
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/React.2EuseCallback/near/1226466
and one conclusion was that that optimization may often not be effective at all. If it is effective here, it's unlikely to be meaningful -- it doesn't seem like these icon components (which are what ultimately get some of these callbacks as props) should have much work at all to do in their render functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants