Clicking outside menus closes them by outoftime · Pull Request #1016 · popcodeorg/popcode

outoftime · 2017-09-04T14:31:06Z

The proximate/user-facing effect of this change is that clicking outside open menus closes them, which is an expected/good UX pattern.

Toward that end, we refactor the top bar menus to use a common component factory, createMenu. This looks something like a higher-order component, although I’m not sure it really qualifies as an HOC because the component passed into the HOC is just the label, whereas the output component is a fully-functional menu.

In the process of this refactor, I also discovered that using the components/index and containers/index modules to provide named exports of components/containers, while aesthetically pleasing, causes circular dependencies which result in imported components that are undefined at the time of module evaluation. This hasn’t been an issue when imported components are only dereferenced in render functions, but when creating a higher-order component, we pass a component at module evaluation time, so it can’t be undefined. Anyway, the behavior of the module loader in this situation is deeply unintuitive (I spent several hours trying to figure out what was going on).

So, introduces the webpack-circular-dependencies plugin and eliminates the usage of the index modules, which were the cause of the only circular dependencies in our code. The plugin will cause compilation to fail if circular dependencies are introduced in the future.

Fixes #1003

While I like this from a code organization standpoint, there is a subtle issue where containers require components require containers, such that a circular dependency can be created around `containers/index`. This causes named imports from `containers` to be `undefined` at module evaluation time, which hasn’t come up before, but is coming up now. Since the pattern is primarily aesthetic, just remove the index modules and require the component modules directly.

Circular dependencies in WebPack can cause imported modules to be `undefined` at module evaluation time, which is unexpected and hard to debug. Instead, just throw an error if a circular dependency is detected.

* Introduces `react-click-outside` to wrap components and give them click outside handling * Adds this component to the `<CurrentUserMenu>` component * Introduces a `CLOSE_TOP_BAR_MENU` action that is dispatched when `CLICK_OUTSIDE` is called * Generalizes the logic for suppressing pointer events in the output—now this will happen if we are currently dragging the column divider, or if there is a top bar menu open. The latter allows the top-level `<body>` to capture clicks that would otherwise be swallowed by the preview iframe

Builds a component factory for top bar menus, which looks vaguely like a higher-order component but doesn’t really fit the pattern. Specifically: * The `createMenu()` function takes an object with properties `name` and `mapPropsToItems`, which returns a function… * That takes two component classes, a `Label` class and an `Item` class. This will then return a highly decorated component that renders the menu, deals with opening/closing, marking enabled items as enabled, etc. One of the decorations is the `onClickOutside` HOC which properly handles clicks outside the menu (closing it).

Ports the project picker (formerly project list) component to use createMenu, giving it proper click-outside behavior, etc. Required enhancing the `createMenu` factory to take an optional `isVisible` function, which takes in props and determines whether the menu should appear at all.

Initial implementation had `createMenu` take a `mapPropsToItems`, which returned an array of objects containing properties `key`, `isEnabled`, and `props`. The higher-order component was then passed a component class which actually rendered items. As well as setting up a structure that’s both rigid and not particularly discoverable, this meant that menus were assumed to consist of a homogeneous collection of items, each of which is rendered in the same way and behaves in the same way when clicked. This is not true, however, of things like the hamburger menu. So, instead, `createMenu` exports a `<MenuItem>` component, which is used to wrap the actual contents of a menu item and takes `key`, `isEnabled`, and `onClick` props. The factory now takes a `renderItems` function, which can return any valid JSX node. This also improves the interface of generated menu components, since the click handler can now be called whatever feels natural.

Makes less use of `<MenuItem>` because the functionality is fairly bespoke

Final installation of porting all menus to `createMenu`, the current user “menu”. One change is that `createMenu` needs to pass the props to `<Label>` which seems entirely reasonable.

Updates test plan to match current UI/feature set.

inlinestyle · 2017-09-06T15:33:44Z

+    onExportGist,
+    onExportRepo,
+  }) {
+    return tap([], (items) => {


I'm curious, are you using tap here to make it clear from the top that items is what we'll be returning? Or is there some other functional purpose?

Yeah, maybe because I’ve written a lot of Ruby where it’s a very common pattern, but my instinct is usually to tap when the situation is “create some object/data structure, mutate it a bunch, and then return it”. Does it feel awkward to you?

inlinestyle · 2017-09-06T15:51:08Z

+    };
+  }
+
+  return function createMenuWithMappedProps(Label) {


A name like LaunchButton or MenuLaunchButton or MenuLauncher might be a little more helpful for figuring out what this argument is for (assuming that I've got it figured out 😑).

GC! Certainly instructive that all the actual components that get passed in here are named with the pattern *Button

inlinestyle · 2017-09-06T15:53:11Z

+      </MenuItem>
+    ));
+  },
+})(ProjectPickerButton, ProjectPreview);


Hmm does the second argument get used? It looks like these args go to createMenuWithMappedProps, which seems to have an arity of 1.

OOPS YOU ARE RIGHT

(My initial implementation took a second component argument which was used to render items, but that proved insufficiently flexible)

inlinestyle · 2017-09-06T15:55:28Z

  URL](http://localhost:3000/?gist=339c841617fb50c98420d9f37654039d) and ensure
  that the gist is imported into the environment
+* Open [a gist with
+  instructions](http://localhost:3000/?gist=911a82a17a280545858d2d8ecc557ef3).


Oh hey that's me! Note to self: update these instructions with an example of a gist that has non-included syntax in #1000.

outoftime force-pushed the click-outside branch from 3bcd167 to ea16d28 Compare September 4, 2017 14:31

Mat Brown added 4 commits September 4, 2017 11:12

Install circular dependency plugin

8b32d87

Circular dependencies in WebPack can cause imported modules to be `undefined` at module evaluation time, which is unexpected and hard to debug. Instead, just throw an error if a circular dependency is detected.

outoftime force-pushed the click-outside branch from ea16d28 to bbcdf03 Compare September 4, 2017 15:12

Mat Brown added 5 commits September 4, 2017 12:11

Hamburger menu uses createMenu

d269a9f

Makes less use of `<MenuItem>` because the functionality is fairly bespoke

Current user menu uses createMenu

3db933f

Final installation of porting all menus to `createMenu`, the current user “menu”. One change is that `createMenu` needs to pass the props to `<Label>` which seems entirely reasonable.

Update test plan

ce2a301

Updates test plan to match current UI/feature set.

outoftime force-pushed the click-outside branch from d0cc53c to ce2a301 Compare September 4, 2017 16:12

outoftime merged commit 3603fc8 into popcodeorg:master Sep 4, 2017

outoftime deleted the click-outside branch September 4, 2017 16:46

inlinestyle reviewed Sep 6, 2017

View reviewed changes

outoftime mentioned this pull request Sep 7, 2017

Code improvements to click-outside #1023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clicking outside menus closes them#1016

Clicking outside menus closes them#1016
outoftime merged 9 commits intopopcodeorg:masterfrom
outoftime:click-outside

outoftime commented Sep 4, 2017

Uh oh!

inlinestyle Sep 6, 2017

Uh oh!

outoftime Sep 7, 2017

Uh oh!

inlinestyle Sep 6, 2017

Uh oh!

outoftime Sep 7, 2017

Uh oh!

inlinestyle Sep 6, 2017

Uh oh!

outoftime Sep 7, 2017

Uh oh!

outoftime Sep 7, 2017

Uh oh!

inlinestyle Sep 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

outoftime commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants