8000 Inject requirejs in notebook and start using it. by Carreau · Pull Request #3364 · ipython/ipython · GitHub
[go: up one dir, main page]

Skip to content

Inject requirejs in notebook and start using it. #3364

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

Merged
merged 4 commits into from
Jun 7, 2013

Conversation

Carreau
Copy link
Member
@Carreau Carreau commented May 26, 2013

Not sure if we want to inject requirejs in one step everywhere or step-by-step in multiple PR each of which would refactor only a few files.

@ellisonbg
Copy link
Member

I was thinking about when we want to make this transition. We have a
lot to do before the 1.0 release and this will be pretty major work
that would block some of the more pressing things. @fperez and @minrk
what do you think - should we try to do this now or wait until after
1.0? This is important work, but feature wise it doesn't have to be
done before 1.0.

On Sun, May 26, 2013 at 12:44 PM, Matthias Bussonnier
notifications@github.com wrote:

Not sure if we want to inject requirejs in one step everywhere or
step-by-step in multiple PR each of which would refactor only a few files.


You can merge this Pull Request by running

git pull https://github.com/Carreau/ipython requirejs

Or view, comment on, or merge it at:

#3364

Commit Summary

Inject requirejs in notebook and start using it.

File Changes

M IPython/frontend/html/notebook/static/components (2)
M IPython/frontend/html/notebook/static/notebook/js/config.js (14)
M IPython/frontend/html/notebook/static/notebook/js/main.js (26)
M IPython/frontend/html/notebook/templates/notebook.html (2)
M IPython/frontend/html/notebook/templates/page.html (7)

Patch Links:

https://github.com/ipython/ipython/pull/3364.patch
https://github.com/ipython/ipython/pull/3364.diff

@Carreau
Copy link
Member Author
Carreau commented May 26, 2013

I would agree not to do it before 1.0, whic is too much work. But having require.js will help a lot for extensions. By trying to put it into the main code base (just for testing purpose) I saw that it was breaking marked. So this mean that people who would like to inject requirejs for plugin could actually break the notebook which is annoying.

So I'm not proposing to do it 100%, but at least have it injected plus the minimal modification to be sure things are not broken with it.

@minrk
Copy link
Member
minrk commented May 27, 2013

I'm not sure I understand what the decision is here - it sounds like we don't want to merge this for 1.0, but there is talk of something partial, and I don't see what that partial step is.

@Carreau
Copy link
Member Author
Carreau commented May 27, 2013

No, by this I mean, completely move all modules to require.js , which is obviously to much work.

What I propose is to add require into the component submodules, add in to the loaded script, and fixed what need to be fixed for marked to work.

Basically this PR plus a few stuff to avoid attaching marked to window

@minrk
Copy link
Member
minrk commented May 27, 2013

so this PR attaches marked to the window, but then you want to do another PR to undo that?

@Carreau
Copy link
Member Author
Carreau commented May 28, 2013

No, I want to improve it by passing marked around, or required it in
textcell.js directly, I just didn't had the time to do it. And just issued
a pr to discuss it.

@Carreau
Copy link
Member Author
Carreau commented Jun 4, 2013

Still in progress, would like to clean/squash a little before merging if people still agree.
To come back at what was discussed in the hangout.

  • require is injected in page.html (all pages) I think it makes sens as custom.js is also, it is before marked
  • I could put it later in the page, but we would have to move it earlier to start using it if we ever decide so.

TODO:

  • I have to update the submodule

// as injecting require.js make marked not to put itself in the globals,
// which make both this file fail at setting marked configuration, and textcell.js
// which search marked into global.
require(['static/components/marked/lib/marked.js'],
Copy link
Member

Choose a reason for hiding this comment

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

this must be allowed to be static_url as in the HTML templates

Copy link
Member Author

Choose a reason for hiding this comment

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

Done by setting require search root as {{static_url()}} in templates.

@minrk
Copy link
Member
minrk commented Jun 4, 2013

Nice, thanks. If you have it all squared away with submodules hash, etc., I'm 👍 for merge now.

@Carreau
Copy link
Member Author
Carreau commented Jun 4, 2013

I'd like to play with it a bit. especially re-read the doc of require and its configuration.
I'll try to do it for next Hangout.

@minrk
Copy link
Member
minrk commented Jun 4, 2013

works for me.

@fperez
Copy link
Member
fperez commented Jun 6, 2013

We'll make sure this goes in before attempting to merge #3395 (or its successor if we end up redoing it from scratch).

@Carreau
Copy link
Member Author
Carreau commented Jun 7, 2013

If it does not make it (I'll try to play with it today to be sure) it would not be too painfull to rebase.

@Carreau
Copy link
Member Author
Carreau commented Jun 7, 2013

Works nicely, also with min's bootstrap. Works also great with custom.js where you can use stuff like:

require(['custom/slidemode/main'])

Merging ! Thanks.

Carreau added a commit that referenced this pull request Jun 7, 2013
Inject requirejs in notebook and start using it.

Mainly because the behavior of Marked change when require is injected.
So only apply the modification needed for marked to behave.
@Carreau Carreau merged commit 3db168c into ipython:master Jun 7, 2013
@Carreau Carreau deleted the requirejs branch June 7, 2013 08:41
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Inject requirejs in notebook and start using it.

Mainly because the behavior of Marked change when require is injected.
So only apply the modification needed for marked to behave.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0