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

Create a shortcut editor for the notebook. #1347

Merged
merged 3 commits into from May 5, 2016

Conversation

Projects
None yet
3 participants
@Carreau
Member

Carreau commented Apr 13, 2016

Replace #1265

Also introduce es6, and write the new keyboard shortcut editor as pure es6.
Does not use react.


[edit]
Update description after lots of refactor.

Create shortcut editor for the notebook

  1. finish the step allowing the use of es6
  • this include some tweak to web pack configuration to speed up
    recompile in watch mode (in particular cache sourcemaps).
  • enable eslint (error only), on obvious mistakes.
  • setup babel to compile to es5 as a target.
  1. Make the test pass under Casper that does not always have
    Function.prototype.bind defined, which we cannot patch only in the
    tests.

  2. Write an actual shortcut editor that list and allow to modify most of
    the command mode shortcut.

The logic to persist the shortcuts is a bit tricky as there are default
keyboard shortcuts, and so when you "unbind" them you need to re-unbind
them at next startup. This does not work for a few shortcut for
technical reasons: <Esc>, <Shift>, as well as <Ctrl-Shift-P> and <F>
which register asynchronously, so are not detected as "default"
shortcuts.

@sccolbert

This comment has been minimized.

Show comment
Hide comment
@sccolbert

sccolbert Apr 13, 2016

Contributor

Should preact be a runtime dependency instead of a dev dependency?

Contributor

sccolbert commented Apr 13, 2016

Should preact be a runtime dependency instead of a dev dependency?

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 13, 2016

Member

Should preact be a runtime dependency instead of a dev dependency?

Likely yes, reflect of --save-dev.

Member

Carreau commented Apr 13, 2016

Should preact be a runtime dependency instead of a dev dependency?

Likely yes, reflect of --save-dev.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 13, 2016

Member

reflex*,

and it will be bundled in the min.js that will generated by webpack and be shipped, so not actually not sure.

Member

Carreau commented Apr 13, 2016

reflex*,

and it will be bundled in the min.js that will generated by webpack and be shipped, so not actually not sure.

Show outdated Hide outdated package.json
Show outdated Hide outdated package.json
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 14, 2016

Member

ping @jdfreder , you're the one that know the most things about notebook test. Test seem to not run at all with this PR, but I can't seem to figure out the reason why, as the same step works flawlessly locally.

Any ideas ?

Member

Carreau commented Apr 14, 2016

ping @jdfreder , you're the one that know the most things about notebook test. Test seem to not run at all with this PR, but I can't seem to figure out the reason why, as the same step works flawlessly locally.

Any ideas ?

Carreau added some commits Apr 18, 2016

Create shortcut editor for the notebook
1) finish the step allowing the use of es6

  - this include some tweak to web pack configuration to speed up
    recompile in watch mode (in particular cache sourcemaps).
  - enable eslint (error only), on obvious mistakes.
  - setup babel to compile to es5 as a target.

2) Make the test pass under Casper that does not always have
`Function.prototype.bind` defined, which we cannot patch only in the
tests.

3) Write an actual shortcut editor that list and allow to modify most of
the command mode shortcut.

The logic to persist the shortcuts is a bit tricky as there are default
keyboard shortcuts, and so when you "unbind" them you need to re-unbind
them at next startup. This does not work for a few shortcut for
technical reasons: `<Esc>`, `<Shift>`, as well as `<Ctrl-Shift-P>` and `<F>`
which register asynchronously, so are not detected as "default"
shortcuts.
@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 29, 2016

Member

BTW, this now works, and can now use es6.

Member

Carreau commented Apr 29, 2016

BTW, this now works, and can now use es6.

@sccolbert

This comment has been minimized.

Show comment
Hide comment
@sccolbert

sccolbert Apr 29, 2016

Contributor

What do you mean by "can use es6"?

Contributor

sccolbert commented Apr 29, 2016

What do you mean by "can use es6"?

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 29, 2016

Member

What do you mean by "can use es6"?

Sorry, was unclear, you can use es6 feature in all the js files (typically notebook/static/*/js/*.js), webpack use the a babel loader when minifying, so will understand let, const, import {} from ... etc. There is no extra steps.

Member

Carreau commented Apr 29, 2016

What do you mean by "can use es6"?

Sorry, was unclear, you can use es6 feature in all the js files (typically notebook/static/*/js/*.js), webpack use the a babel loader when minifying, so will understand let, const, import {} from ... etc. There is no extra steps.

@sccolbert

This comment has been minimized.

Show comment
Hide comment
@sccolbert

sccolbert Apr 29, 2016

Contributor

Ah - I didn't realize we were using babel. Are we still targeting ES5 runtimes?

Contributor

sccolbert commented Apr 29, 2016

Ah - I didn't realize we were using babel. Are we still targeting ES5 runtimes?

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 29, 2016

Member

Are we still targeting ES5 runtimes?

Yes. We can use something else than babel, I guess we can use a typescript loader as well, I have no issues with that.

One thing I did not include is the ability to use .jsx, but that should be pretty easy to add too if we decide to do so.

Member

Carreau commented Apr 29, 2016

Are we still targeting ES5 runtimes?

Yes. We can use something else than babel, I guess we can use a typescript loader as well, I have no issues with that.

One thing I did not include is the ability to use .jsx, but that should be pretty easy to add too if we decide to do so.

@sccolbert

This comment has been minimized.

Show comment
Hide comment
@sccolbert

sccolbert Apr 29, 2016

Contributor

I don't think we need to use the TS loader at this point. We've been doing okay just shipping the built .js files for our code which is written in TS.

Contributor

sccolbert commented Apr 29, 2016

I don't think we need to use the TS loader at this point. We've been doing okay just shipping the built .js files for our code which is written in TS.

@sccolbert

This comment has been minimized.

Show comment
Hide comment
@sccolbert

sccolbert Apr 29, 2016

Contributor

Also, I think (but could be wrong) that preact has an h() function, which can reduce the boilerplate needed by createElement.

Contributor

sccolbert commented Apr 29, 2016

Also, I think (but could be wrong) that preact has an h() function, which can reduce the boilerplate needed by createElement.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau Apr 29, 2016

Member

Also, I think (but could be wrong) that preact has an h() function, which can reduce the boilerplate needed by createElement.

Yes it has a h() function, though they don't really explain how/why to use it, expect using /** @jsx h */ in jsx files, and it looks mostly as s/createElement/h/g, but I might be missing something.

Member

Carreau commented Apr 29, 2016

Also, I think (but could be wrong) that preact has an h() function, which can reduce the boilerplate needed by createElement.

Yes it has a h() function, though they don't really explain how/why to use it, expect using /** @jsx h */ in jsx files, and it looks mostly as s/createElement/h/g, but I might be missing something.

@Carreau

This comment has been minimized.

Show comment
Hide comment
@Carreau

Carreau May 5, 2016

Member

Ok, merging to get people to complain.

Member

Carreau commented May 5, 2016

Ok, merging to get people to complain.

@Carreau Carreau merged commit d8fc951 into jupyter:master May 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Carreau Carreau deleted the Carreau:shortcut-editor-2 branch May 5, 2016

@minrk minrk added this to the 5.0 milestone May 30, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment