Skip to content
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

Merged
merged 18 commits into from Jan 31, 2018

Conversation

Projects
None yet
4 participants
@giladgray
Copy link
Contributor

commented Jan 30, 2018

Fixes #849, Fixes #1640, Fixes #2008

Changes proposed in this pull request:

  • ⭐️ submenus automatically flip to left side if there is not enough room onscreen.
  • submenus will reposition vertically to fit onscreen, rather than forcing the screen to scroll
  • 🔥 remove MenuItem useSmartPositioning and submenuViewportMargin props, and all associated logic, as Popper.js beautifully handles flipping when a menu reaches the edge.
    • also remove the tests cuz it's all in Popper now (#2008)
    • this functionality had zero usages that I could find
    • Popper's logic can be overridden through MenuItem popoverProps.modifiers

Screenshot

notice how the menu never leaves the viewport (or causes it to scroll #849)!!

submenu-flipping

Gilad Gray added some commits Jan 30, 2018

Gilad Gray
Add submenuBoundaryElement, submenuBoundaryPadding props to Menu. cas…
…cade to MenuItem via context fn (better than cloning children).
Gilad Gray
@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented on f678135 Jan 30, 2018

docs

Preview: documentation | landing | table

Gilad Gray

@giladgray giladgray requested review from llorca, cmslewis and adidahiya Jan 30, 2018

@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2018

more docs

Preview: documentation | landing | table

@giladgray giladgray changed the title Gg/submenus Submenu flipping with Popper.js! Jan 30, 2018

@llorca

This comment has been minimized.

Copy link
Contributor

commented Jan 30, 2018

🔥

Can you bring back the original alignment of submenus? It's off by 5px in both directions
screen shot 2018-01-29 at 8 37 00 pm

getSubmenuPopperModifiers: assertFunctionProp,
};

// simple alternative to prop-types dependency

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 30, 2018

Member

I don't see the point to this -- we're already depending on prop-types implicitly through react. You should use the package directly and also remove this code:

// simple alternative to prop-types dependency

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

well dang, i didn't realize react depended on it!

blueprint git:(gg/submenus) yarn why prop-types
yarn why v1.3.2
[1/4] 🤔  Why do we have the module "prop-types"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
info Reasons this module exists
   - "@blueprintjs/core#react-popper" depends on it
   - "@blueprintjs/core#react" depends on it
   - "@blueprintjs/core#react-transition-group" depends on it

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

no-implicit-dependency-wise, is prop-types a new peerDep, alongside react?

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 30, 2018

Member

no, it's a regular dependency, not a peerDep.

`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
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.

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 30, 2018

Member

If it's an anti-pattern, why even mention it?

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

yeah i had this exact thought. i figure it's going to happen (is already happening) so why not pass judgment on it.

@@ -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

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 30, 2018

Member

When would I need to customize these things? Genuinely curious.

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

you might want to keep your Menu inside a container other than the viewport, maybe on a canvas. padding i think is less useful and less likely to be used so i could be convinced to remove it, but it's easy enough to support.

}
// NOTE: use .pt-menu directly because using Menu would start a new context tree and
// we'd have to pass through the submenu props. this is just simpler.
const submenuContent = <ul className={Classes.MENU}>{this.renderChildren()}</ul>;

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 30, 2018

Member

if this.renderChildren() returns null, wouldn't you want to avoid rendering this <ul> and <Popover> altogether?

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

hmm yes, makes sense! surprising that never came up.

private renderPopover(content: JSX.Element) {
const { disabled, popoverProps } = this.props;
const popoverClasses = classNames(Classes.MENU_SUBMENU, popoverProps.popoverClassName);
// getSubmenuPopperModifiers will not be defined if `MenuItem` used outside a `Menu`.

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 30, 2018

Member

Should we throw an error in that case?

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

🤔 i'm hesitant to throw errors in cases where it's not absolutely fatal. pretty easy to handle the no-context case here.

submenuLeft += adjustmentWidth;
submenuRight += adjustmentWidth;
}
// NOTE: use .pt-menu directly because using Menu would start a new context tree and

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 30, 2018

Member

What do you mean by new context tree? And by "the submenu props" do you mean submenuBoundaryElement and submenuBoundaryPadding?

This feels dirty, I'd rather use <Menu>. You can derive the submenu info you need from props and context in the component constructor.

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

new context tree: the nested Menu overwrites the same context key with a function that returns its default props, not the parent's. by not using Menu, the root context gets passed all the way down.

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 30, 2018

Author Contributor

it's doable with Menu, i'll update the usage so you can see. don't need to do any constructor stuff, i can just pass parent props down.

@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2018

use Menu and pass context props

Preview: documentation | landing | table

@adidahiya

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

  • We should remove submenuBoundaryPadding as a prop. Please don't add it just because it's "easy enough to support" -- IMO nothing ever is. We can add it back later if users ask for it.
  • We should also strongly consider removing submenuBoundaryElement until users ask for it. Can you demonstrate a legit use case for that here in this PR (ideally in the docs example)?
@@ -4,21 +4,12 @@
* Licensed under the terms of the LICENSE file distributed with this project.
*/

import { isFunction } from "../../common/utils";
import { func } from "prop-types";

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 31, 2018

Member

not a fan of this import, the symbols are too terse. for example another possible import is any, which would be very confusing to import. please switch to import * as PropTypes

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 31, 2018

Author Contributor

yeah i felt the terseness. went with this for theoretical tree-shaking purposes, but i agree the * as import makes a lot more sense here.

import {
IBlock,
IKssPluginData,
IMarkdownPluginData,
ITsDocBase,
ITypescriptPluginData,
} from "documentalist/dist/client";
import { func } from "prop-types";

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 31, 2018

Member

same here, use namespace import

});
const { disabled, label } = this.props;
const submenuChildren = this.renderSubmenuChildren();
const hasSubmenu = submenuChildren != null;

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 31, 2018

Member

nit: only used once, can be inlined

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 31, 2018

Author Contributor

used twice, or i would have already inlined it 😄 (other usage is hidden in collapsed block, line 66)

@giladgray

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2018

@adidahiya ok, i'm onboard to remove the two props. the only real benefit they conferred was the cascading behavior to all nested submenus. the behavior is still entirely possible through popoverProps.modifier on MenuItem.

@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2018

remove submenu props docs

Preview: documentation | landing | table

@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2018

Merge branch 'develop' of github.com:palantir/blueprint into gg/submenus

Preview: documentation | landing | table

Gilad Gray added some commits Jan 31, 2018

Gilad Gray
@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2018

delete menu test

Preview: documentation | landing | table

@giladgray

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2018

#2058 should fix test failures here.

@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2018

Merge branch 'develop' of github.com:palantir/blueprint into gg/submenus

Preview: documentation | landing | table

/** Props to spread to `Popover`. Note that `content` cannot be changed. */
popoverProps?: Partial<IPopoverProps> & object;
/** Props to spread to `Popover`. Note that `content` and `minimal` cannot be changed. */
popoverProps?: Partial<IPopoverProps>;

This comment has been minimized.

Copy link
@adidahiya

adidahiya Jan 31, 2018

Member

& object was there intentionally... IIRC without it, you can assign literally anything non-null to this field.

This comment has been minimized.

Copy link
@giladgray

giladgray Jan 31, 2018

Author Contributor

& object messes up the docs, which i take to be more important. a prop called *Props is expected to be an object.
screen shot 2018-01-31 at 3 06 45 pm

@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2018

button menu items inherit font

Preview: documentation | landing | table

@blueprint-bot

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2018

improve Overlay focus tests reliability

Preview: documentation | landing | table

@giladgray giladgray merged commit 208f7a8 into develop Jan 31, 2018

7 checks passed

ci/circleci: compile Your tests passed on CircleCI!
Details
ci/circleci: deploy-preview Your tests passed on CircleCI!
Details
ci/circleci: dist Your tests passed on CircleCI!
Details
ci/circleci: install-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
cla/palantir CLA signed via membership in "palantir"
Details

@giladgray giladgray deleted the gg/submenus branch Jan 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.