-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Migrate components/post_view/post_flag_icon/post_flag_icon.tsx
from class to function component
#24187
Conversation
const PostFlagIcon = ({ | ||
actions: { | ||
flagPost, | ||
unflagPost, | ||
}, | ||
isFlagged, | ||
postId, | ||
location = Locations.CENTER, | ||
}: 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.
Expanded props to better track unused props.
const buttonRef = useRef<HTMLButtonElement>(null); | ||
const [a11yActive, setA11yActive] = useState(false); | ||
|
||
const handlePress = useCallback((e: React.MouseEvent) => { |
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.
useCallback
to avoid passing a new function to children objects on every render.
const handleA11yActivateEvent = () => { | ||
setA11yActive(true); | ||
}; | ||
|
||
handleA11yDeactivateEvent = () => { | ||
this.setState({a11yActive: false}); | ||
const handleA11yDeactivateEvent = () => { | ||
setA11yActive(false); | ||
}; |
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.
No need for useCallback
since are only used on on mount/dismount
); | ||
}; | ||
|
||
export default React.memo(PostFlagIcon); |
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.
Rect.memo
since the component used to be a Pure Component.
|
||
handleA11yActivateEvent = () => { | ||
this.setState({a11yActive: true}); | ||
const handleA11yActivateEvent = () => { |
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 this 2 functions should be defined inside the useEffect
, that is the right scope for them, and there is not reason to have it outside.
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 have a tiny improvement suggestion, but otherwise looks good to me.
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
const handleA11yActivateEvent = () => { | ||
setA11yActive(true); | ||
}; | ||
const handleA11yDeactivateEvent = () => { | ||
setA11yActive(false); | ||
}; |
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.
Any reasons why these are anonymous functions stored in a variable instead of plain functions?
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.
If you mean why use const foo = () => {...}
instead of using just () => {...}
where needed, it is because I want to send the same reference to both add and remove listener.
If you mean why use const foo = () => {...}
instead of func foo () {...}
, no reason at all. Not sure if there is any real difference between both options.
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 i meant latter, there is no difference anyways. We don't want to set expectations for contributors to always use anonymous functions instead of plain functions.
@@ -19,115 +19,97 @@ export type Actions = { | |||
unflagPost: typeof unflagPost; | |||
} | |||
|
|||
interface Props { | |||
type 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.
why the change 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.
I was receiving a weird typescript error (maybe just on local) where the index file was complaining with the following error: Default export of the module has or is using private name 'Props'.
. Moving it to a type fixed 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.
hmm thats interesting
@M-ZubairAhmed The failing test seems to be non related to this. Any chance it is a flaky test? |
Yes this is a flaky test, happens with my PRs randomly sometimes as well
|
E2E tests not automatically triggered, because the PR is not in a mergeable state. Please update the branch with the base branch and resolve outstanding conflicts. |
@yasserfaraazkhan QA Notes: The No functionality should have changed. |
/update-branch |
Summary
Migrate
components/post_view/post_flag_icon/post_flag_icon.tsx
from class to function componentTicket Link
None
Release Note