Skip to content
This repository has been archived by the owner on Jun 13, 2022. It is now read-only.

Plugin doesn't work with Nuclide #3

Closed
ehellman opened this issue Feb 24, 2017 · 12 comments
Closed

Plugin doesn't work with Nuclide #3

ehellman opened this issue Feb 24, 2017 · 12 comments
Assignees

Comments

@ehellman
Copy link

I'm having the same issue as another person who's ticket is now closed, it just started working for him again after a while.

As requested, I looked in dev tools and my active pane does not have the hey-pane-focus class. Flex-grow changes when I resize the pane manually.

I've tried switching the keybinding to something else, but it just won't fire.

Here is a list of my installed packages:
https://gist.githubusercontent.com/ehellman/6ccf288440808c1c2da2c205baa11530/raw/4eb50972689f6175be44b6d72861a3ae1db1331f/packages.json

@timomeh
Copy link
Owner

timomeh commented Feb 24, 2017

Hm, strange. It sounds like the the plugin trigger doesn't get called at all, If even executing it through the Command Palette doesn't work. I'll make some research if that's a known problem for atom packages, and how to fix this.

In the meantime, could you please specify your Atom Version (the output of atom --version), just to be super sure?

@ehellman
Copy link
Author

ehellman commented Feb 24, 2017

❯ atom --version
Atom    : 1.14.3
Electron: 1.3.13
Chrome  : 52.0.2743.82
Node    : 6.5.0

I'd love to help to investigate this, I'll be on a few hour long train ride tomorrow and will look into the plugin code, maybe I'll find something else I can contribute to as well! :)

@timomeh
Copy link
Owner

timomeh commented Feb 24, 2017

@ehellman Great, of course you can! You could test if the toggleFocus() method gets called at all:

toggleFocus() {

That's the function which will be called when running the command, see:

'hey-pane:toggle-focus-of-active-pane': () => this.toggleFocus(),

@ehellman
Copy link
Author

ehellman commented Mar 3, 2017

Reporting back, Nuclide is the culprit here. Haven't figured out why yet though, but here is an interesting clue:

When I log out the classList of this.ActivePane from doFocus() the array does contain .hey-pane-focused, HOWEVER, when I inspect the DOM, the class is not displayed on the element, just .pane and .active is visible.

I don't think I will solve this without looking into what Nuclide does with panes, but it might be something you want to add to the README or something until it is resolved, so that people know that the plugin works, but that it is incompatible with that package.

Works great when Nuclide is disabled! :)

By the way, the shadows in your demo looks great, what do you do to make your Atom look like that?

@timomeh
Copy link
Owner

timomeh commented Mar 3, 2017

Aaaah, Nuclide, okay. Nuclide is bloatware very extensive. (I don't want to bad-mouth Nuclide, I tried it and saw it's advantages, but didn't stick with it).

I'll add a note to the README.

I don't want to close this issue now, I'll try to investigate this, too – but I can't promise I'll have more success than you.

Thank you very much for your research, it really helped me a lot.


Oh, yeah, the shadow/darkened tree-view is pretty dope. I ripped it from @brumm. (Shameless advertising, you should really check out the stuff he's programming. He's also the reason why we have the awesome custom title bar for Atom in macOS.) Add this to your styles.less:

.tree-view-resizer {
  position: relative;
  &:after {
    content: '';
    position: absolute;
    top: 0;
    right: 0;
    height: 100%;
    width: 10px;
    background-image: linear-gradient(
      90deg,
      transparent,
      rgba(0, 0, 0, 0.2)
    );
    z-index: 1;
  }
}

.tree-view {
  transition: opacity 150ms;
  &:not(:hover) {
    opacity: 0.2;
  }
}

@timomeh timomeh changed the title Shift-Cmd-K does not focus active pane, neither does the menu item Plugin doesn't work with Nuclide Mar 3, 2017
@brumm
Copy link

brumm commented Mar 3, 2017

@ehellman here's the lastest iteration of that little sidebar hack

.tree-view-scroller > ol > li > ol > li {
  padding-left: 0 !important;
}

.tree-view-resizer {
  width: 100% !important;
  min-width: 0;

  &:hover {
    transition: all 0ms linear 100ms;
  }
  &:not(:hover) {
    opacity: 0.2;
    width: 13px !important;
  }

  position: relative;
  &:after {
    content: '';
    position: absolute;
    top: 0;
    right: 0;
    height: 100%;
    width: 10px;
    background-image: linear-gradient(
      90deg,
      transparent,
      rgba(0, 0, 0, 0.1)
    );
    z-index: 1;
  }
}

.tree-view {
  .project-root-header {
    display: none;
  }
}

@brumm
Copy link

brumm commented Mar 3, 2017

@timomeh maybe you could add some kind of compatibility warning to your package, like language-babel does in this commit: gandm/language-babel@0069b92

@timomeh
Copy link
Owner

timomeh commented Mar 3, 2017

@brumm Ha! Didn't know that's even a thing. Yep, will definitely add this in the next days.

@ehellman
Copy link
Author

ehellman commented Mar 6, 2017

Thanks for the tip @brumm, I got the newer snippet working with standard Atom, however Nuclide breaks that one too. I ended up removing Nuclide (used it for working sets).

@ehellman
Copy link
Author

ehellman commented Mar 6, 2017

I don't know when I am going to have more time to debug this, but when I get time I will check back and install Nuclide again to see if I can fix it and put the fix in a PR. I imagine I have to look a bit at the Nuclide code first though to see how it handles panes.

@timomeh
Copy link
Owner

timomeh commented Mar 6, 2017

It seems like Nuclide behaves very strangely.

Try running the following command in the DevTools Console of Atom (with Nuclide), which should resize the active pane:

atom.views.getView(atom.workspace).querySelector('.pane.active').style.flexGrow=2;

Nothing will happen. Without Nuclide, it'll work. But if you run this:

var activePane = atom.views.getView(atom.workspace).querySelector('.pane.active');
activePane.style.flexGrow=2;

... it will work. But I don't know why, because in my understanding of JavaScript, this shouldn't make a difference.

Nevertheless, this doesn't mean that it's easily fixable. I tried a few things and they didn't work. I don't want to implement some weird things, which I'll have to maintain in the future, just to support Nuclide.

I hereby announce that this Package is incompatible with Nuclide and that I won't fix this. 😄

If you or someone else has a simple solution to this, feel free to open a PR.

@timomeh
Copy link
Owner

timomeh commented Apr 19, 2017

You won't believe it, but there are no problems with Nuclide anymore. I don't know if that was fixed by Nuclide, by Atom itself or through my enhancements with v0.3.0.

Thus I removed the incompatibility message.

@timomeh timomeh closed this as completed Apr 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants