-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Fix popover placement when there is no enough space on top or bottom of content #303
Changes from 4 commits
765470d
1fe0012
07e7a52
74aa7df
901c29d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 ( | ||
<ListSpacingContext.Provider value={false}> | ||
<div | ||
|
@@ -55,7 +58,10 @@ function Popover({ | |
{...otherProps} | ||
> | ||
<span className={BEM.arrow} style={arrowStyle} /> | ||
<div className={BEM.container}> | ||
<div | ||
className={BEM.container} | ||
style={{ maxHeight: remainingSpace ? remainingSpace - popoverPadding : undefined }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 有點隱晦,覺得可以一起放進註解 |
||
> | ||
{children} | ||
</div> | ||
</div> | ||
|
@@ -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 }; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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} | ||
Comment on lines
+34
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice tests 👍 |
||
`; | ||
|
||
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); | ||
} | ||
); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
return TOP; | ||
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); | ||
Comment on lines
-63
to
+80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 確保當 placement 為 TOP 時,Popover 最高只會被擺到畫面上緣。如果是負的有可能會看不到 Popover 上半部。 |
||
} 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, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
覺得 popoverPadding 可以移到 function 外宣告
然後直接在這裡算 maxHeight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
也可,style 比較乾淨