Change class components to functional components
#5431
Conversation
🦋 Changeset detectedLatest commit: 4dcb007 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4dcb007:
|
nderkim
force-pushed
the
alex/refactor-class-components-to-functional
branch
from
29198dc to
0ca2d5b
Compare
| const getPortalPlacement = useCallback( | ||
| ({ placement: updatedPlacement }: CalculatedMenuPlacementAndHeight) => { | ||
| // avoid re-renders if the placement has not changed | ||
| if (updatedPlacement !== placement) { |
There was a problem hiding this comment.
I don't think we need this check because React knows to bail out if the value did not change.
| exited: { width: 0 }, | ||
| }; | ||
| nodeRef = createRef<HTMLDivElement>(); | ||
| export const Collapse = ({ children, in: _in, onExited }: CollapseProps) => { |
There was a problem hiding this comment.
I assume that in is from "react-transition-group"—reserved word for a prop was such a bad idea...
| {(state) => ( | ||
| <div | ||
| ref={ref} | ||
| style={{ |
There was a problem hiding this comment.
Nit: Could the nested ternaries be against an object with state keys, or some getStyleFromState fn with a switch?
| @@ -55,10 +55,10 @@ interface PlacementArgs { | |||
| } | |||
|
|
|||
| export function getMenuPlacement({ | |||
| maxHeight, | |||
| maxHeight: desiredMaxHeight, | |||
There was a problem hiding this comment.
Glad these are now differentiated! I think "desired" isn't quite right, though. Maybe preferredX or consumerX?
| placement: CoercedMenuPlacement | null; | ||
| maxHeight: number; | ||
| } | ||
| const PortalPlacementContext = |
There was a problem hiding this comment.
Declaring undefined feels weird. Could this be:
createContext<CtxType | undefined>()
// OR
createContext<CtxType | null>(null)
| const [placement, setPlacement] = useState<CoercedMenuPlacement | null>(null); | ||
|
|
||
| useLayoutEffect(() => { | ||
| const { current } = ref; |
There was a problem hiding this comment.
Nit: destructuring doesn't offer a lot of value here, prefer meaningful variable name e.g.
useLayoutEffect(() => {
const menuEl = ref.current
if (!menuEl) return
const state = getMenuPlacement({
menuEl,
...
})
})
| } | ||
| } | ||
| return ( | ||
| <Fragment> |
There was a problem hiding this comment.
I get why this is here, but I think it's a mistake in our types 🤔 For another day:
export interface MenuPlacerProps {
- children: (childrenProps: ChildrenProps) => ReactNode
+ children: (childrenProps: ChildrenProps) => ReactElement
}
| @@ -690,7 +684,7 @@ export const MenuPortal = < | |||
| ); | |||
|
|
|||
| return ( | |||
| <PortalPlacementContext.Provider value={{ getPortalPlacement }}> | |||
| <PortalPlacementContext.Provider value={{ setPortalPlacement }}> | |||
There was a problem hiding this comment.
State setters are equal between renders, but the object will be created each time. Might be worth memoizing to reduce consumer re-renders?
const context = useMemo(() => ({ setPortalPlacement }), [setPortalPlacement])
return <PortalPlacementContext.Provider value={context} />
No description provided.