-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Submenu flipping with Popper.js! #2053
Changes from 4 commits
fe8b321
46fe754
f678135
aef958e
e23f78c
a796819
b770995
2e92f5a
9800ef2
c7bd0ac
85f2356
8ee3917
12958ee
774173e
4b92799
472361d
c9f43ca
cc9af72
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 |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* | ||
* Copyright 2018 Palantir Technologies, Inc. All rights reserved. | ||
* | ||
* Licensed under the terms of the LICENSE file distributed with this project. | ||
*/ | ||
|
||
import { isFunction } from "../../common/utils"; | ||
|
||
export interface IMenuItemContext { | ||
getSubmenuPopperModifiers?(): Popper.Modifiers; | ||
} | ||
|
||
export const MenuItemContextTypes: React.ValidationMap<IMenuItemContext> = { | ||
getSubmenuPopperModifiers: assertFunctionProp, | ||
}; | ||
|
||
// simple alternative to prop-types dependency | ||
function assertFunctionProp<T>(obj: T, key: keyof T) { | ||
// context method is optional, so allow nulls | ||
if (obj[key] == null || isFunction(obj[key])) { | ||
return undefined; | ||
} | ||
return new Error(`[Blueprint] context ${key} must be function. received ${typeof obj[key]}.`); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,25 +83,34 @@ To add a submenu to a `Menu`, simply nest `MenuItem`s within another `MenuItem`. | |
The submenu opens to the right of its parent by default, but will adjust and flip to the left if | ||
there is not enough room to the right. | ||
|
||
`Menu` provides two `submenu` props to adjust this flipping behavior: you can customize the boundary element | ||
and the padding within that boundary element. Note that a `MenuItem` _must be inside_ a `Menu` 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. When would I need to customize these things? Genuinely curious. 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. you might want to keep your |
||
for these props to affect the submenus. On standalone `MenuItem`s (without a parent `Menu`, an anti-pattern) you can | ||
use `popoverProps.modifiers` directly to achieve the same result. | ||
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. If it's an anti-pattern, why even mention it? 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. yeah i had this exact thought. i figure it's going to happen (is already happening) so why not pass judgment on it. |
||
|
||
```jsx | ||
<MenuItem text="Submenu"> | ||
<MenuItem text="Child one" /> | ||
<MenuItem text="Child two" /> | ||
<MenuItem text="Child three" /> | ||
</MenuItem> | ||
<Menu> | ||
<MenuItem text="Submenu"> | ||
<MenuItem text="Child one" /> | ||
<MenuItem text="Child two" /> | ||
<MenuItem text="Child three" /> | ||
</MenuItem> | ||
</Menu> | ||
``` | ||
|
||
Alternatively, you can pass an array of `IMenuItemProps` to the `submenu` prop: | ||
|
||
```jsx | ||
React.createElement(MenuItem, { | ||
submenu: [ | ||
{ text: "Child one" }, | ||
{ text: "Child two" }, | ||
{ text: "Child three" }, | ||
], | ||
text: "parent", | ||
}); | ||
React.createElement(Menu, {}, | ||
React.createElement(MenuItem, { | ||
submenu: [ | ||
{ text: "Child one" }, | ||
{ text: "Child two" }, | ||
{ text: "Child three" }, | ||
], | ||
text: "parent", | ||
}), | ||
); | ||
``` | ||
|
||
<div class="pt-callout pt-intent-warning pt-icon-warning-sign"> | ||
|
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.
I don't see the point to this -- we're already depending on
prop-types
implicitly throughreact
. You should use the package directly and also remove this code:blueprint/packages/docs-theme/src/common/context.ts
Line 71 in 02c4a98
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.
well dang, i didn't realize react depended on it!
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.
no-implicit-dependency-wise, is
prop-types
a new peerDep, alongsidereact
?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.
no, it's a regular dependency, not a peerDep.