From 765470d9c1ca2dd44a2e7c6730f9bb46d006a860 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=B3=E4=B9=99=E5=B1=B1?= Date: Wed, 9 Dec 2020 13:48:09 +0800 Subject: [PATCH 1/5] Let anchor placement be bottom when there is no enough space for content --- packages/core/src/mixins/anchored/getPositionState.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/mixins/anchored/getPositionState.js b/packages/core/src/mixins/anchored/getPositionState.js index 5be31cf8..78c74455 100644 --- a/packages/core/src/mixins/anchored/getPositionState.js +++ b/packages/core/src/mixins/anchored/getPositionState.js @@ -41,7 +41,7 @@ export function getPlacement(defaultPlacement, anchorRectTop, anchorHeight, self if (defaultPlacement === TOP && !hasSpaceToPlaceSelfAbove) { return BOTTOM; } - if (defaultPlacement === BOTTOM && !hasSpaceToPlaceSelfBelow) { + if (defaultPlacement === BOTTOM && !hasSpaceToPlaceSelfBelow && hasSpaceToPlaceSelfAbove) { return TOP; } From 1fe001210b0d2625ba6760ae871a06cb34791bf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=B3=E4=B9=99=E5=B1=B1?= Date: Wed, 9 Dec 2020 14:01:49 +0800 Subject: [PATCH 2/5] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5b5c5a3..d4408b32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - [Core] Upgrade `eslint-config-ichef` packages to `v8.0.0`. (#290) - [Core] Use node 12 in travis CI. (#290) - [Core] Add `titleRightArea` on `
` / ``. (#297) +- [Core] Fix popover placement when there is no enough space on top or bottom of content. (#303) ### Added - [Core] Add `muted` prop on following component: (#278) From 07e7a528bbb16015080e3e7f6372db5d94aececb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=B3=E4=B9=99=E5=B1=B1?= Date: Wed, 9 Dec 2020 16:21:50 +0800 Subject: [PATCH 3/5] Handle placement of anchored when there's no space --- packages/core/src/Popover.js | 10 +++++- .../src/mixins/anchored/getPositionState.js | 34 ++++++++++++++----- packages/core/src/mixins/anchored/index.js | 3 ++ 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/packages/core/src/Popover.js b/packages/core/src/Popover.js index a298bf9b..a8a69500 100644 --- a/packages/core/src/Popover.js +++ b/packages/core/src/Popover.js @@ -30,6 +30,7 @@ function Popover({ placement, arrowStyle, nodeRef, + remainingSpace, // from closable() onInsideClick, // React props @@ -45,6 +46,8 @@ function Popover({ onClick(event); }; + const popoverPadding = 24; + return (
-
+
{children}
@@ -68,6 +74,7 @@ Popover.propTypes = { placement: anchoredPropTypes.placement, arrowStyle: anchoredPropTypes.arrowStyle, nodeRef: anchoredPropTypes.nodeRef, + remainingSpace: anchoredPropTypes.remainingSpace, onInsideClick: PropTypes.func.isRequired, }; @@ -76,6 +83,7 @@ Popover.defaultProps = { placement: ANCHORED_PLACEMENT.BOTTOM, arrowStyle: {}, nodeRef: undefined, + remainingSpace: undefined, }; export { Popover as PurePopover }; diff --git a/packages/core/src/mixins/anchored/getPositionState.js b/packages/core/src/mixins/anchored/getPositionState.js index 78c74455..2ca26d07 100644 --- a/packages/core/src/mixins/anchored/getPositionState.js +++ b/packages/core/src/mixins/anchored/getPositionState.js @@ -31,21 +31,37 @@ export const PLACEMENT = { TOP, BOTTOM }; * @param {number} anchorRectTop * @param {number} anchorHeight * @param {number} selfHeight + * @returns {{ placement: Placement, remainingSpace: number }} */ -export function getPlacement(defaultPlacement, anchorRectTop, anchorHeight, selfHeight) { +export function getPlacementAndRemainingSpace( + defaultPlacement, + anchorRectTop, + anchorHeight, + selfHeight +) { const hasSpaceToPlaceSelfAbove = anchorRectTop >= selfHeight; const hasSpaceToPlaceSelfBelow = ( (anchorRectTop + anchorHeight + selfHeight) <= window.innerHeight ); - + const topSpace = anchorRectTop; + const bottomSpace = window.innerHeight - anchorRectTop - anchorHeight; + if (!hasSpaceToPlaceSelfBelow && !hasSpaceToPlaceSelfAbove) { + return { + placement: topSpace > bottomSpace ? TOP : BOTTOM, + remainingSpace: topSpace > bottomSpace ? topSpace : bottomSpace, + }; + } if (defaultPlacement === TOP && !hasSpaceToPlaceSelfAbove) { - return BOTTOM; + return { placement: BOTTOM, remainingSpace: bottomSpace }; } - if (defaultPlacement === BOTTOM && !hasSpaceToPlaceSelfBelow && hasSpaceToPlaceSelfAbove) { - return TOP; + if (defaultPlacement === BOTTOM && !hasSpaceToPlaceSelfBelow) { + return { placement: TOP, remainingSpace: topSpace }; } - return defaultPlacement; + return { + placement: defaultPlacement, + remainingSpace: defaultPlacement === TOP ? topSpace : bottomSpace, + }; } /** @@ -60,7 +76,8 @@ export function getTopPosition(placement, anchorOffsetTop, anchorHeight, selfHei let positionTop = 0; if (placement === TOP) { - positionTop = anchorOffsetTop - selfHeight; + // Make sure user can see whole wrapped component when placement is TOP. + positionTop = Math.max(anchorOffsetTop - selfHeight, 0); } else { positionTop = anchorOffsetTop + anchorHeight; } @@ -184,7 +201,7 @@ const getPositionState = (defaultPlacement, edgePadding) => (anchorNode, selfNod // Determine position // ------------------------------------- - const placement = getPlacement( + const { placement, remainingSpace } = getPlacementAndRemainingSpace( defaultPlacement, anchorRect.top, anchorRect.height, @@ -208,6 +225,7 @@ const getPositionState = (defaultPlacement, edgePadding) => (anchorNode, selfNod return { placement, + remainingSpace, position: { top: selfTop, left: selfLeft, diff --git a/packages/core/src/mixins/anchored/index.js b/packages/core/src/mixins/anchored/index.js index b2d8bfad..b38c795c 100644 --- a/packages/core/src/mixins/anchored/index.js +++ b/packages/core/src/mixins/anchored/index.js @@ -15,6 +15,7 @@ export const anchoredPropTypes = { placement: PropTypes.oneOf(Object.values(PLACEMENT)), arrowStyle: PropTypes.objectOf(PropTypes.number), nodeRef: PropTypes.func, + remainingSpace: PropTypes.number.isRequired, }; function filterDOMNode(node) { @@ -127,6 +128,7 @@ const anchored = ({ placement, position, arrowPosition, + remainingSpace, } = this.getPositions( filterDOMNode(anchor), filterDOMNode(selfNode), @@ -142,6 +144,7 @@ const anchored = ({ Date: Wed, 9 Dec 2020 17:11:36 +0800 Subject: [PATCH 4/5] Fix test of getPositionState --- .../__tests__/getPositionState.test.js | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/packages/core/src/mixins/anchored/__tests__/getPositionState.test.js b/packages/core/src/mixins/anchored/__tests__/getPositionState.test.js index ff514aae..d002c36d 100644 --- a/packages/core/src/mixins/anchored/__tests__/getPositionState.test.js +++ b/packages/core/src/mixins/anchored/__tests__/getPositionState.test.js @@ -1,7 +1,7 @@ import documentOffset from 'document-offset'; import getPositionState, { - getPlacement, + getPlacementAndRemainingSpace, getTopPosition, getLeftPositionSet, PLACEMENT, @@ -31,23 +31,28 @@ jest.mock('document-offset', () => ( describe('getPlacement()', () => { const { TOP, BOTTOM } = PLACEMENT; const runTest = it.each` - expected | defaultVal | situation | anchorTop | anchorHeight | selfHeight - ${TOP} | ${TOP} | ${'enough'} | ${120} | ${30} | ${100} - ${BOTTOM} | ${TOP} | ${'not enough'} | ${90} | ${30} | ${100} - ${TOP} | ${BOTTOM} | ${'not enough'} | ${600} | ${100} | ${100} - ${BOTTOM} | ${BOTTOM} | ${'enough'} | ${300} | ${100} | ${100} + expected | defaultVal | situation | anchorTop | anchorHeight | selfHeight | remainingSpace + ${TOP} | ${TOP} | ${'enough'} | ${120} | ${30} | ${100} | ${120} + ${BOTTOM} | ${TOP} | ${'not enough for top'} | ${90} | ${30} | ${100} | ${648} + ${TOP} | ${BOTTOM} | ${'not enough for bottom'} | ${600} | ${100} | ${100} | ${600} + ${BOTTOM} | ${BOTTOM} | ${'enough'} | ${300} | ${100} | ${100} | ${368} + ${BOTTOM} | ${BOTTOM} | ${'not enough for both, but bottom is larger'} | ${300} | ${100} | ${400} | ${368} + ${TOP} | ${BOTTOM} | ${'not enough for both, but top is larger'} | ${450} | ${100} | ${500} | ${450} `; runTest( - 'returns $expected when default is $defaultVal, and there is $situation space', - ({ expected, defaultVal, anchorTop, anchorHeight, selfHeight }) => { - const result = getPlacement( + 'returns $expected when default is $defaultVal, and the space is $situation', + ({ + expected, defaultVal, anchorTop, anchorHeight, selfHeight, remainingSpace, + }) => { + const result = getPlacementAndRemainingSpace( defaultVal, anchorTop, anchorHeight, selfHeight, ); - expect(result).toBe(expected); + expect(result.placement).toBe(expected); + expect(result.remainingSpace).toBe(remainingSpace); } ); }); From 901c29d1e30c473ccafffdb83cdebdbbeedc8863 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=99=B3=E4=B9=99=E5=B1=B1?= Date: Thu, 10 Dec 2020 12:06:41 +0800 Subject: [PATCH 5/5] Add comment for Popover maxHeight calculation --- packages/core/src/Popover.js | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/core/src/Popover.js b/packages/core/src/Popover.js index a8a69500..0297b01c 100644 --- a/packages/core/src/Popover.js +++ b/packages/core/src/Popover.js @@ -24,6 +24,8 @@ export const BEM = { container: ROOT_BEM.element('container'), }; +const POPOVER_PADDING = 24; + function Popover({ onClick, // from anchored() @@ -40,14 +42,19 @@ function Popover({ }) { const bemClass = BEM.root.modifier(placement); const rootClassName = classNames(bemClass.toString(), className); + /** + * The `remainingSpace` is the space for whole popover. + * What we want here is to always show keep `remainingSpace === popoverHeight` + * The `maxHeight` is for `BEM.container`, which doesn't include root class padding. + * So we need to minus POPOVER_PADDING here. + */ + const maxHeight = remainingSpace ? remainingSpace - POPOVER_PADDING : undefined; const handleWrapperClick = (event) => { onInsideClick(event); onClick(event); }; - const popoverPadding = 24; - return (
{children}