-
Notifications
You must be signed in to change notification settings - Fork 25
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 DropdownMenu
component issues
#2804
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
583fa31
fix(dropdown-menu): fix max height management
CarlosCortizasCT d97df5d
fix(dropdown-menu): fix scrolling issues
CarlosCortizasCT e50ae54
feat(dropdown-menu): introducing the new menuMaxHeight property
CarlosCortizasCT 9dbc6c6
chore: changeset added
CarlosCortizasCT File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
--- | ||
'@commercetools-uikit/dropdown-menu': minor | ||
--- | ||
|
||
We've fixed two issues we had regarding floating menu position when scrolling and its height. | ||
|
||
We're also introducing a new property (`menuMaxHeight`) which allows consumers to limit the floating panel maximum height. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
import React, { | ||
import { | ||
useCallback, | ||
useEffect, | ||
useMemo, | ||
useRef, | ||
type ReactElement, | ||
type ReactNode, | ||
type RefObject, | ||
} from 'react'; | ||
import { useToggleState } from '@commercetools-uikit/hooks'; | ||
import { type TMaxProp } from '@commercetools-uikit/constraints'; | ||
|
@@ -21,6 +23,11 @@ export type TDropdownMenuProps = { | |
* The position of the menu relative to the trigger element. | ||
*/ | ||
menuPosition?: 'left' | 'right'; | ||
/** | ||
* The maximum height for the menu in pixels. | ||
* By default, the max height will be the available space between the trigger element and the bottom of the viewport. | ||
*/ | ||
menuMaxHeight?: number; | ||
/** | ||
* The element that triggers the dropdown. | ||
*/ | ||
|
@@ -40,6 +47,48 @@ export type TDropdownMenuProps = { | |
children: ReactNode; | ||
}; | ||
|
||
function getScrollableParent(element: HTMLElement | null): HTMLElement | null { | ||
if (!element) { | ||
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. We want to block the scroll of the nearest ancestor with scrolling enabled, if any. |
||
return null; | ||
} | ||
const overflowY = window.getComputedStyle(element).overflowY; | ||
const isScrollable = overflowY !== 'visible' && overflowY !== 'hidden'; | ||
if (isScrollable && element.scrollHeight >= element.clientHeight) { | ||
return element; | ||
} | ||
|
||
return getScrollableParent(element.parentElement); | ||
} | ||
|
||
function useScrollBlock(isOpen: boolean, triggerRef: RefObject<HTMLElement>) { | ||
const scrollableParentRef = useRef<HTMLElement | null>(); | ||
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. Just to extract a bit of logic from the component itself. |
||
|
||
useEffect(() => { | ||
if (!scrollableParentRef.current) { | ||
scrollableParentRef.current = getScrollableParent(triggerRef.current); | ||
} | ||
|
||
const { current: scrollableParent } = scrollableParentRef; | ||
if (scrollableParent && isOpen) { | ||
scrollableParent.setAttribute( | ||
'data-prev-scroll', | ||
scrollableParent.style.overflowY | ||
); | ||
scrollableParent.style.overflowY = 'hidden'; | ||
} | ||
return () => { | ||
// The cleanup effect runs after the component is unmounted but also everytime | ||
// the dependency array changes. We need to manage both to manage opening/closing | ||
// the dropdown but also to manage the the dropdown is opened and the component | ||
// is unmounted. For instance, when navigating to another page client-side. | ||
if (scrollableParent && isOpen) { | ||
const prevScroll = scrollableParent.getAttribute('data-prev-scroll'); | ||
scrollableParent.style.overflowY = prevScroll || ''; | ||
} | ||
}; | ||
}, [isOpen, scrollableParentRef, triggerRef]); | ||
} | ||
|
||
const defaultProps: Pick< | ||
TDropdownMenuProps, | ||
'menuPosition' | 'menuType' | 'menuHorizontalConstraint' | ||
|
@@ -56,7 +105,7 @@ const Container = styled.div` | |
|
||
function DropdownMenu(props: TDropdownMenuProps) { | ||
const [isOpen, toggle] = useToggleState(false); | ||
const triggerRef = React.useRef<HTMLDivElement>(null); | ||
const triggerRef = useRef<HTMLDivElement>(null); | ||
|
||
// We use the context so children can toggle the dropdown | ||
const context = useMemo( | ||
|
@@ -91,17 +140,10 @@ function DropdownMenu(props: TDropdownMenuProps) { | |
window.removeEventListener('click', handleGlobalClick); | ||
}; | ||
}, [handleGlobalClick]); | ||
// Block scrolling when the dropdown is open | ||
useEffect(() => { | ||
if (isOpen) { | ||
window.document.body.style.overflow = 'hidden'; | ||
} else { | ||
window.document.body.style.overflow = 'initial'; | ||
} | ||
return () => { | ||
window.document.body.style.overflow = 'initial'; | ||
}; | ||
}, [isOpen]); | ||
|
||
// Block scrolling when the dropdown is open. | ||
// We do this to avoid requiring dropdown rerendering while the user scrolls. | ||
useScrollBlock(isOpen, triggerRef); | ||
|
||
return ( | ||
<DropdownMenuContext.Provider value={context}> | ||
|
@@ -114,6 +156,7 @@ function DropdownMenu(props: TDropdownMenuProps) { | |
horizontalConstraint={props.menuHorizontalConstraint!} | ||
isOpen={isOpen} | ||
menuPosition={props.menuPosition!} | ||
menuMaxHeight={props.menuMaxHeight} | ||
triggerElementRef={triggerRef} | ||
> | ||
{props.children} | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is to allow having long lists in the dropdown menu so the height and scrollability can be tested.
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.
💯