From eb789a0ab15cffa83c6585011ddf27aa79ad8c60 Mon Sep 17 00:00:00 2001 From: Juho Heikka Date: Tue, 21 Sep 2021 23:53:11 +0300 Subject: [PATCH 1/2] Fix menu rendering on when slightly off screen Menu would bounce around in some cases because the refreshPosition() would calculate different results based on different state of the component and previous render cycle CSS classes. Also I found that the render function calls causes in an infinite loop while open because of refreshPosition will cause a state change which will rerender the function. Fixed that with the shouldComponentUpdate which checks if state has actually changed in between renders. Signed-off-by: Juho Heikka --- src/renderer/components/menu/menu.scss | 8 ---- src/renderer/components/menu/menu.tsx | 54 +++++++++++++++----------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/src/renderer/components/menu/menu.scss b/src/renderer/components/menu/menu.scss index d94cbff63e..0db62e278a 100644 --- a/src/renderer/components/menu/menu.scss +++ b/src/renderer/components/menu/menu.scss @@ -35,14 +35,6 @@ &.portal { left: -1000px; top: -1000px; - - &.top { - margin-top: -$margin; - } - - &.bottom { - margin-top: $margin; - } } &:not(.portal) { diff --git a/src/renderer/components/menu/menu.tsx b/src/renderer/components/menu/menu.tsx index 30780e7ac4..8ceef6bf8e 100644 --- a/src/renderer/components/menu/menu.tsx +++ b/src/renderer/components/menu/menu.tsx @@ -26,6 +26,7 @@ import { createPortal } from "react-dom"; import { autoBind, cssNames, noop } from "../../utils"; import { Animate } from "../animate"; import { Icon, IconProps } from "../icon"; +import isEqual from "lodash/isEqual"; export const MenuContext = React.createContext(null); export type MenuContextValue = Menu; @@ -85,6 +86,10 @@ export class Menu extends React.Component { return !!this.props.isOpen; } + shouldComponentUpdate(nextProps: MenuProps, nextState: State) { + return !isEqual(nextState, this.state) || !isEqual(nextProps, this.props); + } + componentDidMount() { if (!this.props.usePortal) { const parent = this.elem.parentElement; @@ -150,39 +155,42 @@ export class Menu extends React.Component { return; } - const { width, height } = this.opener.getBoundingClientRect(); - let { left, top, bottom, right } = this.opener.getBoundingClientRect(); + const openerClientRect = this.opener.getBoundingClientRect(); + let { left: openerLeft, top: openerTop, bottom: openerBottom, right: openerRight } = this.opener.getBoundingClientRect(); const withScroll = window.getComputedStyle(this.elem).position !== "fixed"; // window global scroll corrections if (withScroll) { - left += window.pageXOffset; - top += window.pageYOffset; - right = left + width; - bottom = top + height; + openerLeft += window.pageXOffset; + openerTop += window.pageYOffset; + openerRight = openerLeft + openerClientRect.width; + openerBottom = openerTop + openerClientRect.height; } - // setup initial position - const position: MenuPosition = { left: true, bottom: true }; + const extraMargin = this.props.usePortal ? 8 : 0; - this.elem.style.left = `${left}px`; - this.elem.style.top = `${bottom}px`; + const { width: menuWidth, height: menuHeight } = this.elem.getBoundingClientRect(); - // correct position if menu doesn't fit to viewport - const menuPos = this.elem.getBoundingClientRect(); + const rightSideOfMenu = openerLeft + menuWidth; + const renderMenuLeft = rightSideOfMenu > window.innerWidth; + const menuOnLeftSidePosition = `${openerRight - this.elem.offsetWidth}px`; + const menuOnRightSidePosition = `${openerLeft}px`; - if (menuPos.right > window.innerWidth) { - this.elem.style.left = `${right - this.elem.offsetWidth}px`; - position.right = true; - delete position.left; - } + this.elem.style.left = renderMenuLeft ? menuOnLeftSidePosition : menuOnRightSidePosition; - if (menuPos.bottom > window.innerHeight) { - this.elem.style.top = `${top - this.elem.offsetHeight}px`; - position.top = true; - delete position.bottom; - } - this.setState({ position }); + const bottomOfMenu = openerBottom + extraMargin + menuHeight; + const renderMenuOnTop = bottomOfMenu > window.innerHeight; + const menuOnTopPosition = `${openerTop - this.elem.offsetHeight - extraMargin}px`; + const menuOnBottomPosition = `${openerBottom + extraMargin}px`; + + this.elem.style.top = renderMenuOnTop ? menuOnTopPosition : menuOnBottomPosition; + + this.setState({ position: { + top: renderMenuOnTop, + bottom: !renderMenuOnTop, + left: renderMenuLeft, + right: !renderMenuLeft + } }); }; open() { From b70ec5ddbd9cade0fbdaa3d3510b2f3d7e118c54 Mon Sep 17 00:00:00 2001 From: Juho Heikka Date: Mon, 27 Sep 2021 17:03:27 +0300 Subject: [PATCH 2/2] Remove shouldComponentUpdate, put style to state instead of straight to DOM so they will be in sync Signed-off-by: Juho Heikka --- src/renderer/components/menu/menu.tsx | 52 ++++++++++++++++----------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/src/renderer/components/menu/menu.tsx b/src/renderer/components/menu/menu.tsx index 8ceef6bf8e..4dfddc978d 100644 --- a/src/renderer/components/menu/menu.tsx +++ b/src/renderer/components/menu/menu.tsx @@ -38,6 +38,10 @@ export interface MenuPosition { bottom?: boolean; } +export interface MenuStyle { + top: string; + left: string; +} export interface MenuProps { isOpen?: boolean; open(): void; @@ -57,6 +61,7 @@ export interface MenuProps { interface State { position?: MenuPosition; + menuStyle?: MenuStyle } const defaultPropsMenu: Partial = { @@ -76,7 +81,6 @@ export class Menu extends React.Component { super(props); autoBind(this); } - public opener: HTMLElement; public elem: HTMLUListElement; protected items: { [index: number]: MenuItem } = {}; @@ -86,10 +90,6 @@ export class Menu extends React.Component { return !!this.props.isOpen; } - shouldComponentUpdate(nextProps: MenuProps, nextState: State) { - return !isEqual(nextState, this.state) || !isEqual(nextProps, this.props); - } - componentDidMount() { if (!this.props.usePortal) { const parent = this.elem.parentElement; @@ -124,6 +124,12 @@ export class Menu extends React.Component { window.removeEventListener("scroll", this.onScrollOutside, true); } + componentDidUpdate(prevProps: MenuProps) { + if (!isEqual(prevProps.children, this.props.children)) { + this.refreshPosition(); + } + } + protected get focusableItems() { return Object.values(this.items).filter(item => item.isFocusable); } @@ -176,21 +182,23 @@ export class Menu extends React.Component { const menuOnLeftSidePosition = `${openerRight - this.elem.offsetWidth}px`; const menuOnRightSidePosition = `${openerLeft}px`; - this.elem.style.left = renderMenuLeft ? menuOnLeftSidePosition : menuOnRightSidePosition; - const bottomOfMenu = openerBottom + extraMargin + menuHeight; const renderMenuOnTop = bottomOfMenu > window.innerHeight; const menuOnTopPosition = `${openerTop - this.elem.offsetHeight - extraMargin}px`; const menuOnBottomPosition = `${openerBottom + extraMargin}px`; - this.elem.style.top = renderMenuOnTop ? menuOnTopPosition : menuOnBottomPosition; - - this.setState({ position: { - top: renderMenuOnTop, - bottom: !renderMenuOnTop, - left: renderMenuLeft, - right: !renderMenuLeft - } }); + this.setState({ + position: { + top: renderMenuOnTop, + bottom: !renderMenuOnTop, + left: renderMenuLeft, + right: !renderMenuLeft + }, + menuStyle: { + top: renderMenuOnTop ? menuOnTopPosition : menuOnBottomPosition, + left: renderMenuLeft ? menuOnLeftSidePosition : menuOnRightSidePosition, + } + }); }; open() { @@ -284,10 +292,6 @@ export class Menu extends React.Component { } render() { - if (this.isOpen) { - setImmediate(() => this.refreshPosition()); - } - const { position, id } = this.props; let { className, usePortal } = this.props; @@ -313,7 +317,15 @@ export class Menu extends React.Component { const menu = ( -
    +
      {menuItems}