chore: migrate several class-based components to functional components #5584
Conversation
| private animatedValue: Animated.Value; | ||
| const Dropdown = ({ currentDepartment, onClose, onDepartmentSelected, departments }: IDropdownProps) => { | ||
| const animatedValue = useRef(new Animated.Value(0)).current; | ||
| const { theme } = useTheme(); |
There was a problem hiding this comment.
theme is deprecated on useTheme.
We've been using colors, just like
| const heightDestination = 0; | ||
| const maxRows = 5; |
There was a problem hiding this comment.
Constants should live outside the component, so they won't be recreated.
app/views/DirectoryView/Options.tsx
Outdated
| @@ -26,73 +26,75 @@ interface IDirectoryOptionsProps { | |||
| theme: TSupportedThemes; | |||
| } | |||
|
|
|||
| export default class DirectoryOptions extends PureComponent<IDirectoryOptionsProps, any> { | |||
| private animatedValue: Animated.Value; | |||
| const DirectoryOptions = memo( | |||
There was a problem hiding this comment.
memo requires all props to be memoized in order to work properly.
By default on function components, functions like toggleWorkspace will be recreated on every render, which is going to make memo irrelevant here.
That said,
DirectoryView is a class component and toggleWorkspace uses arrow function, which makes it bound to the class and memo is going to end up working after all.
IMO we should remove memo from here, just because when we migrate DirectoryView to function component, we'll have to be remember to use useCallback on toggleWorkspace there in order to keep memo working here.
DirectoryOptions is not an expensive component, so I say we remove memo.
| const subscription = useRef<Subscription>(); | ||
| const newServerTimeout = useRef<ReturnType<typeof setTimeout> | false>(); | ||
| const isMounted = useRef(false); | ||
|
|
| const isMounted = useRef(false); | ||
|
|
||
| const [servers, setServers] = useState<TServerModel[]>([]); | ||
|
|
| /> | ||
| ); | ||
|
|
||
| const maxRows = 4; |
There was a problem hiding this comment.
Create as MAX_ROWS outside the component
| const [servers, setServers] = useState<TServerModel[]>([]); | ||
|
|
||
| const dispatch = useDispatch(); | ||
| const { theme } = useTheme(); |
There was a problem hiding this comment.
| const { theme } = useTheme(); | |
| const { colors } = useTheme(); |
| ]} | ||
| /> | ||
| </TouchableWithoutFeedback> | ||
| const statusBarHeight = insets?.top ?? 0; |
There was a problem hiding this comment.
Remove this const and use insets directly
| const Dropdown = ({ isMasterDetail, theme, currentFilter, onClose, onFilterSelected }: IDropdownProps) => { | ||
| const animatedValue = useRef(new Animated.Value(0)).current; |
There was a problem hiding this comment.
Remove theme prop and use useTheme
app/views/DirectoryView/Options.tsx
Outdated
| close: onClose, | ||
| changeType, | ||
| toggleWorkspace, | ||
| theme |
There was a problem hiding this comment.
Remove theme prop (on DirectoryView too) and use useTheme
|
@diegolmello I have completed the requested changes |
|
@diegolmello can you check this pr in your free time, thanks |
diegolmello
force-pushed
the
migrate_component_functional
branch
from
a4cd445
to
237d887
Compare
diegolmello
force-pushed
the
migrate_component_functional
branch
from
237d887
to
175d932
Compare
|
Merged. Thanks for your contribution, @jsathu07! |
Proposed changes
The React team recommends replacing class-based components with functional components. This PR migrates several of them.
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments