Skip to content
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.

Sign up for GitHub

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

Change class components to functional components #5431

Merged
merged 4 commits into from
Oct 26, 2022
Merged

Change class components to functional components #5431

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4b14dca
Refactor class components to functional components
nderkim Oct 24, 2022
61f8ee9
Refactor MenuPlacer to functional component
nderkim Oct 26, 2022
0ca2d5b
Add changeset
nderkim Oct 26, 2022
4dcb007
PR review fixes
nderkim Oct 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 79 additions & 85 deletions packages/react-select/src/components/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
/** @jsx jsx */
import {
createContext,
Component,
Fragment,
ReactNode,
RefCallback,
ContextType,
useState,
Ref,
useCallback,
useContext,
useRef,
useState,
} from 'react';
import { jsx } from '@emotion/react';
import { createPortal } from 'react-dom';
Expand Down Expand Up @@ -55,10 +55,10 @@ interface PlacementArgs {
}

export function getMenuPlacement({
maxHeight,
maxHeight: desiredMaxHeight,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Glad these are now differentiated! I think "desired" isn't quite right, though. Maybe preferredX or consumerX?

menuEl,
minHeight,
placement,
placement: desiredPlacement,
shouldScroll,
isFixedPosition,
theme,
Expand All @@ -67,7 +67,7 @@ export function getMenuPlacement({
const scrollParent = getScrollParent(menuEl!);
const defaultState: CalculatedMenuPlacementAndHeight = {
placement: 'bottom',
maxHeight,
maxHeight: desiredMaxHeight,
};

// something went wrong, return default state
Expand Down Expand Up @@ -99,12 +99,12 @@ export function getMenuPlacement({
const scrollUp = scrollTop + menuTop - marginTop;
const scrollDuration = 160;

switch (placement) {
switch (desiredPlacement) {
case 'auto':
case 'bottom':
// 1: the menu will fit, do nothing
if (viewSpaceBelow >= menuHeight) {
return { placement: 'bottom', maxHeight };
return { placement: 'bottom', maxHeight: desiredMaxHeight };
}

// 2: the menu will fit, if scrolled
Expand All @@ -113,7 +113,7 @@ export function getMenuPlacement({
animatedScrollTo(scrollParent, scrollDown, scrollDuration);
}

return { placement: 'bottom', maxHeight };
return { placement: 'bottom', maxHeight: desiredMaxHeight };
}

// 3: the menu will fit, if constrained
Expand All @@ -140,33 +140,33 @@ export function getMenuPlacement({
// 4. Forked beviour when there isn't enough space below

// AUTO: flip the menu, render above
if (placement === 'auto' || isFixedPosition) {
if (desiredPlacement === 'auto' || isFixedPosition) {
// may need to be constrained after flipping
let constrainedHeight = maxHeight;
let constrainedHeight = desiredMaxHeight;
const spaceAbove = isFixedPosition ? viewSpaceAbove : scrollSpaceAbove;

if (spaceAbove >= minHeight) {
constrainedHeight = Math.min(
spaceAbove - marginBottom - spacing.controlHeight,
maxHeight
desiredMaxHeight
);
}

return { placement: 'top', maxHeight: constrainedHeight };
}

// BOTTOM: allow browser to increase scrollable area and immediately set scroll
if (placement === 'bottom') {
if (desiredPlacement === 'bottom') {
if (shouldScroll) {
scrollTo(scrollParent, scrollDown);
}
return { placement: 'bottom', maxHeight };
return { placement: 'bottom', maxHeight: desiredMaxHeight };
}
break;
case 'top':
// 1: the menu will fit, do nothing
if (viewSpaceAbove >= menuHeight) {
return { placement: 'top', maxHeight };
return { placement: 'top', maxHeight: desiredMaxHeight };
}

// 2: the menu will fit, if scrolled
Expand All @@ -175,15 +175,15 @@ export function getMenuPlacement({
animatedScrollTo(scrollParent, scrollUp, scrollDuration);
}

return { placement: 'top', maxHeight };
return { placement: 'top', maxHeight: desiredMaxHeight };
}

// 3: the menu will fit, if constrained
if (
(!isFixedPosition && scrollSpaceAbove >= minHeight) ||
(isFixedPosition && viewSpaceAbove >= minHeight)
) {
let constrainedHeight = maxHeight;
let constrainedHeight = desiredMaxHeight;

// we want to provide as much of the menu as possible to the user,
// so give them whatever is available below rather than the minHeight.
Expand All @@ -209,9 +209,9 @@ export function getMenuPlacement({
// 4. not enough space, the browser WILL NOT increase scrollable area when
// absolutely positioned element rendered above the viewport (only below).
// Flip the menu, render below
return { placement: 'bottom', maxHeight };
return { placement: 'bottom', maxHeight: desiredMaxHeight };
default:
throw new Error(`Invalid placement provided "${placement}".`);
throw new Error(`Invalid placement provided "${desiredPlacement}".`);
}

return defaultState;
Expand Down Expand Up @@ -240,7 +240,7 @@ export interface MenuProps<
> extends CommonPropsAndClassName<Option, IsMulti, Group>,
MenuPlacementProps {
/** Reference to the internal element, consumed by the MenuPlacer component */
innerRef: RefCallback<HTMLDivElement>;
innerRef: Ref<HTMLDivElement>;
innerProps: JSX.IntrinsicElements['div'];
isLoading: boolean;
placement: CoercedMenuPlacement;
Expand All @@ -254,7 +254,7 @@ interface PlacerProps {
}

interface ChildrenProps {
ref: RefCallback<HTMLDivElement>;
ref: Ref<HTMLDivElement>;
placerProps: PlacerProps;
}

Expand Down Expand Up @@ -294,76 +294,81 @@ export const menuCSS = <
zIndex: 1,
});

const PortalPlacementContext = createContext<{
getPortalPlacement:
| ((menuState: CalculatedMenuPlacementAndHeight) => void)
| null;
}>({ getPortalPlacement: null });

interface MenuState {
placement: CoercedMenuPlacement | null;
maxHeight: number;
}
const PortalPlacementContext =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Declaring undefined feels weird. Could this be:

createContext<CtxType | undefined>()

// OR

createContext<CtxType | null>(null)

createContext<
| {
setPortalPlacement: (placement: CoercedMenuPlacement) => void;
}
| undefined
>(undefined);

// NOTE: internal only
export class MenuPlacer<
export const MenuPlacer = <
Option,
IsMulti extends boolean,
Group extends GroupBase<Option>
> extends Component<MenuPlacerProps<Option, IsMulti, Group>, MenuState> {
state: MenuState = {
maxHeight: this.props.maxMenuHeight,
placement: null,
};
static contextType = PortalPlacementContext;
context!: ContextType<typeof PortalPlacementContext>;

getPlacement: RefCallback<HTMLDivElement> = (ref) => {
const {
minMenuHeight,
maxMenuHeight,
menuPlacement,
menuPosition,
menuShouldScrollIntoView,
theme,
} = this.props;
>(
props: MenuPlacerProps<Option, IsMulti, Group>
) => {
const {
children,
minMenuHeight,
maxMenuHeight,
menuPlacement,
menuPosition,
menuShouldScrollIntoView,
theme,
} = props;

if (!ref) return;
const { setPortalPlacement } = useContext(PortalPlacementContext) || {};
const ref = useRef<HTMLDivElement | null>(null);
const [maxHeight, setMaxHeight] = useState(maxMenuHeight);
const [placement, setPlacement] = useState<CoercedMenuPlacement | null>(null);

useLayoutEffect(() => {
const { current } = ref;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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,
    ...
  })
})

if (!current) return;

// DO NOT scroll if position is fixed
const isFixedPosition = menuPosition === 'fixed';
const shouldScroll = menuShouldScrollIntoView && !isFixedPosition;

const state = getMenuPlacement({
maxHeight: maxMenuHeight,
menuEl: ref,
menuEl: current,
minHeight: minMenuHeight,
placement: menuPlacement,
shouldScroll,
isFixedPosition,
theme,
});

const { getPortalPlacement } = this.context;
if (getPortalPlacement) getPortalPlacement(state);

this.setState(state);
};
getUpdatedProps = () => {
const { menuPlacement } = this.props;
const placement = this.state.placement || coercePlacement(menuPlacement);

return { ...this.props, placement, maxHeight: this.state.maxHeight };
};
render() {
const { children } = this.props;
setMaxHeight(state.maxHeight);
setPlacement(state.placement);
setPortalPlacement?.(state.placement);
}, [
maxMenuHeight,
menuPlacement,
menuPosition,
menuShouldScrollIntoView,
minMenuHeight,
setPortalPlacement,
theme,
]);

return children({
ref: this.getPlacement,
placerProps: this.getUpdatedProps(),
});
}
}
return (
<Fragment>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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
}

{children({
ref,
placerProps: {
...props,
placement: placement || coercePlacement(menuPlacement),
maxHeight,
},
})}
</Fragment>
);
};

const Menu = <Option, IsMulti extends boolean, Group extends GroupBase<Option>>(
props: MenuProps<Option, IsMulti, Group>
Expand Down Expand Up @@ -398,7 +403,7 @@ export interface MenuListProps<
/** The children to be rendered. */
children: ReactNode;
/** Inner ref to DOM ReactNode */
innerRef: RefCallback<HTMLDivElement>;
innerRef: Ref<HTMLDivElement>;
/** The currently focused option */
focusedOption: Option;
/** Props to be passed to the menu-list wrapper. */
Expand Down Expand Up @@ -594,23 +599,12 @@ export const MenuPortal = <
const menuPortalRef = useRef<HTMLDivElement | null>(null);
const cleanupRef = useRef<(() => void) | void | null>(null);

const [placement, setPlacement] = useState<'bottom' | 'top'>(
const [placement, setPortalPlacement] = useState<'bottom' | 'top'>(
coercePlacement(menuPlacement)
);
const [computedPosition, setComputedPosition] =
useState<ComputedPosition | null>(null);

// callback for occasions where the menu must "flip"
const getPortalPlacement = useCallback(
({ placement: updatedPlacement }: CalculatedMenuPlacementAndHeight) => {
// avoid re-renders if the placement has not changed
if (updatedPlacement !== placement) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we need this check because React knows to bail out if the value did not change.

setPlacement(updatedPlacement);
}
},
[placement]
);

const updateComputedPosition = useCallback(() => {
if (!controlElement) return;

Expand Down Expand Up @@ -690,7 +684,7 @@ export const MenuPortal = <
);

return (
<PortalPlacementContext.Provider value={{ getPortalPlacement }}>
<PortalPlacementContext.Provider value={{ setPortalPlacement }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

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} />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice catch! 🎉

{appendTo ? createPortal(menuWrapper, appendTo) : menuWrapper}
</PortalPlacementContext.Provider>
);
Expand Down