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

[WIP] Add initial keybindings #3970

Closed
wants to merge 10 commits into from

Conversation

stormpython
Copy link
Member

@stormpython stormpython commented Jan 4, 2019

This PR will add the initial keybindings to nteract.

My initial thoughts are to create a <Keybindings config={{ ... }} /> component that takes a configuration object that specifies which keybindings are associated with certain actions. These are then applied to the appropriate DOM elements.

I will use the blueprintjs Hotkeys component. For more, see here.

This PR should also address these issues or at least provide a framework for implementing the solutions.

@stormpython stormpython force-pushed the keybindings branch 5 times, most recently from f66529a to 3ddf860 Compare January 9, 2019 21:31
@stormpython
Copy link
Member Author

Running into issue with blueprintjs HotkeysTarget decorator component. The first error I was getting was around not being able to use the decorator because it was expecting to call the wrapped component with new.

Uncaught TypeError Class constructor App cannot be invoked without 'new'

This still seems to be an issue, but I've been looking at this issue to help solve it.

Now, I am having an issue getting the event listeners to fire for keydown events. Kyle removed hot module loading to see if it had anything to do with the build, but I am still experiencing issues with getting the event listeners to fire. I am now looking into the blueprintjs HotkeysTarget decorator function to understand how its attaching the event listeners.

@stormpython
Copy link
Member Author

Ok. This is a bigger issue than I thought. The error I am getting when using the HotkeysTarget decorator is still an open issue. In addition, the blueprintjs HotkeysTarget decorator only works when the component returns a DOM element and not a React element, see this issue.

There may be a work around that allows us to get the Hotkeys component working. It involves wrapping the App in a stateless React.PureComponent. If this fails or is to inelegant, I will look into another way to achieve this.

@stormpython
Copy link
Member Author

stormpython commented Jan 11, 2019

Btw, adding experimentalDecorators: true to the tsconfig.json doesn't do the trick either. And after giving the following approach a try (example implementation, not actual):

class StatelessApp extends React.PureComponent {
  // tslint:disable-line max-classes-per-file
  public render() {
    return <App {...{ ...context, settings, noteSettings, hotkeyCallbacks }} />;
  }
}

function AppWrapper() {} // tslint:disable-line no-empty
AppWrapper.prototype = Object.create(StatelessApp.prototype);
AppWrapper.prototype.renderHotkeys = App.hotkeyRenderer(hotkeyCallbacks);
export const AppContainer = HotkeysTarget(AppWrapper as any);

which is a work around for this issue also does not seem to work. I get the following error:

screen shot 2019-01-11 at 9 11 51 am

This occurs because the mapStateToProps doesn't seem to map the keybindings state to the props on initialization.

@rgbkrk
Copy link
Member

rgbkrk commented Jan 11, 2019

Want to table this one for now and I'll take a look at it next week?

@stormpython
Copy link
Member Author

Sure.

@stormpython
Copy link
Member Author

Closing in favor of #4054.

@lock lock bot added the outdated workflow: issue should stay closed label Apr 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated workflow: issue should stay closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants