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

Add context menu to quickly filter bundles (resolves #241) #246

Merged
merged 31 commits into from Apr 10, 2019

Conversation

@bregenspan
Copy link
Contributor

@bregenspan bregenspan commented Jan 28, 2019

As discussed in #241, adding a context menu that exposes options for each chunk can allow for an easier way to trigger certain operations, such as filtering to show only a single chunk, filtering to show all chunks, filtering down to parents of a chunk, or hiding a particular chunk.

This is a first stab at adding such a right-click/Ctrl-click triggered context menu for chunk options, with these options to start with:

  • Hide chunk
  • Show parent chunks (removed for now, see comments)
  • Filter to chunk
  • Show all chunks

One possible limitation of the current approach is that I went for a bespoke context menu based partly on the existing Tooltip component. To add features like a multi-level hierarchy in the context menu in future or more dynamic/context-specific options, it might make more sense to replace with an existing full-featured open-source component.

TODOs:

  • Bug: Clicking outside the context menu closes it. However, the same click event that closes the context menu can cause interaction with the treemap (e.g. a zoom-in on the chunk group that was clicked), which can feel a bit buggy. (Not sure if this is a blocker, but I do want to take another look at fixing).
  • Bug: menu position flickers/shows in upper left briefly upon close (see #246 (comment))
  • Bug: Lack of handling for window resize while menu is open
  • Bug/missing feature: It is possible to hide all chunks until none appear and then the only way to get chunks showing again is to use the sidebar menu or refresh the page
  • When menu is already open, right click/ctrl-click on another chunk when menu is open should immediately open the menu for that chunk, instead of closing the menu and requiring another click to re-open (see #246 (comment))
  • (last tiny one): support opening the menu via event.metaKey in addition to event.ctrlKey?

Possible future cleanup:

  • Better accessibility story? But making the context menu itself screen reader-accessible is pretty clear - the really hard/meaningful part would be getting the treemap itself, and all interactions leading up to the opening of the context menu, to be meaningful! That'd be a pretty big and interesting challenge, probably involves some very thoughtful changes to the actual underlying visualization, so probably makes best sense to address separately.

example screencast

@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented Jan 28, 2019

CLA assistant check
All committers have signed the CLA.

<li className={itemClassName} onClick={this.handleClickHideChunk}>Hide chunk</li>
<li className={itemClassName} onClick={this.handleClickFilterToParents}>Show parent chunks</li>
<li className={itemClassName} onClick={this.handleClickFilterToChunk}>Hide all other chunks</li>
<hr/>
Copy link
Contributor Author

@bregenspan bregenspan Jan 28, 2019

I might have made the laziest possible interpretation of @th0r's request in #241 (comment) to add group separators here.

I could see either abstracting out the ContextMenu component so it can be passed a declarative config of options/option groups OR swapping out in favor of an existing open-source context menu, as there might be one that already has good UX thought put into it and that supports hierarchical menu options/nested menus.

Copy link
Collaborator

@th0r th0r Apr 8, 2019

I could see either abstracting out the ContextMenu component so it can be passed a declarative config of options/option groups

No-no-no, let's keep it like that - it's absolutely fine!

@th0r
Copy link
Collaborator

@th0r th0r commented Jan 28, 2019

  • Clicking outside the context menu closes it. However, the same click event that closes the context menu can cause interaction with the treemap

What about adding a capture-phase click event handler on document and preventing its propagation?

open() {
  // ...
  document.addEventListener('click', event => {
    if (elementIsOutside(event.currentTarget, this.containerElem) {
      event.preventDefault();
      event.stopPropagation();
      this.close();
    }
  }, true);
}

function elementIsOutside(elem, container) {
  return !(elem === container || container.contains(elem));
}

@bregenspan
Copy link
Contributor Author

@bregenspan bregenspan commented Jan 28, 2019

@th0r that did it, thanks -- just pushed that fix up!

(But without the elementIsOutside check, I'll update to use that shortly because I think that does cover a case I missed.)

@bregenspan bregenspan force-pushed the add-context-menu branch from 849e211 to 65c8c03 Apr 3, 2019
@valscion
Copy link
Member

@valscion valscion commented Apr 6, 2019

Hi there @bregenspan, sorry that you've had to wait for so long with this PR.

I took this PR for a spin now and it feels to be working nicely. However, I found myself quite confused on the terminology, and I'm worried that people not so knowledgeable in webpack will have an even harder time to figure out what "parent chunk" means.

At the very least, we would need to document what "parent chunk" means in the readme. How would you describe this to a newcomer?

@bregenspan
Copy link
Contributor Author

@bregenspan bregenspan commented Apr 9, 2019

@th0r I just looked a bit more into whether to add metaKey as a hotkey - is there a particular OS/browser combination where you think it'd be most useful?

Opening context menus via a secondary click triggered by Ctrl feels a bit like a forced fit on Windows, but I think Windows users are more likely to use an actual hardware right click, or are used to the special behavior or Menu key immediately opening an app-specific context menu (which I didn't see working great for this context-specific, not app-wide, case).

Besides that question, I think this might be ready now - all feedback should be addressed, let me know if anything missing. Thanks again @th0r and @valscion for all your time reviewing this!

@th0r
Copy link
Collaborator

@th0r th0r commented Apr 10, 2019

I just looked a bit more into whether to add metaKey as a hotkey

Ok, lets not do it.

th0r
th0r approved these changes Apr 10, 2019
@th0r th0r merged commit 4a232f0 into webpack-contrib:master Apr 10, 2019
2 checks passed
@th0r
Copy link
Collaborator

@th0r th0r commented Apr 10, 2019

Awesome! Thanks a lot!

@th0r
Copy link
Collaborator

@th0r th0r commented Apr 10, 2019

Released in v3.3.0

@bregenspan bregenspan deleted the add-context-menu branch Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants