NotificationsYou must be signed in to change notification settings
Fork5.5k
Star12.8k

Create a shortcut editor for the notebook.#1347

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Carreau merged 3 commits intojupyter:masterfromCarreau:shortcut-editor-2

May 5, 2016

Merged

Create a shortcut editor for the notebook.#1347

Carreau merged 3 commits intojupyter:masterfromCarreau:shortcut-editor-2

May 5, 2016

Conversation

Copy link

Member

Carreau commentedApr 13, 2016•
edited
Loading

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

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.

Make the test pass under Casper that does not always have
Function.prototype.bind defined, which we cannot patch only in the
tests.
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 mentioned this pull request

Apr 13, 2016

[POC] Shortcut editor (for command mode)#1265

Closed

Copy link

Contributor

sccolbert commentedApr 13, 2016

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

Copy link

MemberAuthor

Carreau commentedApr 13, 2016

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

Likely yes, reflect of--save-dev.

Copy link

MemberAuthor

Carreau commentedApr 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.

Carreau force-pushed theshortcut-editor-2 branch from702802b to032b3ffCompare

April 13, 2016 15:14

sccolbert reviewed

Apr 13, 2016

View reviewed changes

package.json Outdated

		"webpack":"^1.12.13"
		},
		"dependencies": {
		"moment":"^2.8.4"

Copy link

Contributor

sccolbertApr 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a syntax error. You need a comma.

Copy link

MemberAuthor

Carreau commentedApr 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 mentioned this pull request

Apr 14, 2016

ES6 and prerequisite to shortcut editor#1351

Merged

Carreau force-pushed theshortcut-editor-2 branch 10 times, most recently from32131cd to831e7cdCompare

April 21, 2016 15:00

Carreau added2 commits

April 21, 2016 18:00

Create shortcut editor for the notebook

ab85f16

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 thetests.3) Write an actual shortcut editor that list and allow to modify most ofthe command mode shortcut.The logic to persist the shortcuts is a bit tricky as there are defaultkeyboard shortcuts, and so when you "unbind" them you need to re-unbindthem at next startup. This does not work for a few shortcut fortechnical reasons: `<Esc>`, `<Shift>`, as well as `<Ctrl-Shift-P>` and `<F>`which register asynchronously, so are not detected as "default"shortcuts.

dynamic sourcemaps

12b4834

Carreau force-pushed theshortcut-editor-2 branch froma4ea097 to12b4834Compare

April 21, 2016 16:01

Add eslint deps

baa49f7

Copy link

MemberAuthor

Carreau commentedApr 29, 2016

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

Copy link

Contributor

sccolbert commentedApr 29, 2016

What do you mean by "can use es6"?

Copy link

MemberAuthor

Carreau commentedApr 29, 2016

What do you mean by "can use es6"?

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

Copy link

Contributor

sccolbert commentedApr 29, 2016

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

Copy link

MemberAuthor

Carreau commentedApr 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.

Copy link

Contributor

sccolbert commentedApr 29, 2016•
edited
Loading

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.

Copy link

Contributor

sccolbert commentedApr 29, 2016

Also, I think (but could be wrong) that preact has anh() function, which can reduce the boilerplate needed bycreateElement.

Copy link

MemberAuthor

Carreau commentedApr 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 ah() function, though they don't really explain how/why to use it, expect using/** @jsx h */ injsx files, and it looks mostly ass/createElement/h/g, but I might be missing something.