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

Dewijmoize #1256

Merged
merged 27 commits into from Jan 13, 2012
Merged

Dewijmoize #1256

merged 27 commits into from Jan 13, 2012

Conversation

ellisonbg
Copy link
Member

A jQueryUI based menu drived UI. Other aspects of the notebook have been improved as well:

  • Save/rename/make a copy logic improved.
  • Last saved time/date now displayed in the header.

Before this can be merged though, we need to fix the printing, which broke in moving to the menu. But please start to review as the printing will be fixed soon.

@ellisonbg
Copy link
Member Author

OK I have fixed printing. I have created a new "Print View" that prints nicely. This needs solid testing on multiple browsers, with and without login enabled.


@authenticate_unless_readonly
def get(self, notebook_id):
nbm = self.application.notebook_manager
Copy link
Member

Choose a reason for hiding this comment

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

I think we should start putting docstrings on these guys. I know in the beginning the whole nb code was so massive that we couldn't be too detailed with this, but before we end up with a ton of undocumented code, having a short explanation on these guys would be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is too early to start documenting things like this. All of these handlers are going to get completely redone in the near future. Don't get me wrong - I do think documenting things like this is important once the code settles down.

Copy link
Member

Choose a reason for hiding this comment

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

No prob.

@fperez
Copy link
Member

fperez commented Jan 12, 2012

This looks great. I've played with it in use, and it's a massive improvement. I'll continue using this as my working branch to test it out more carefully, but it's looking great so far.

Once printing is fixed, I think we can move quickly forward with it. Great job!

@ellisonbg
Copy link
Member Author

Printing IS fixed now...

On Wed, Jan 11, 2012 at 10:45 PM, Fernando Perez
reply@reply.github.com
wrote:

This looks great.  I've played with it in use, and it's a massive improvement.  I'll continue using this as my working branch to test it out more carefully, but it's looking great so far.

Once printing is fixed, I think we can move quickly forward with it.  Great job!


Reply to this email directly or view it on GitHub:
#1256 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Copy link
Member

fperez commented Jan 12, 2012

On Wed, Jan 11, 2012 at 10:45 PM, Brian E. Granger
reply@reply.github.com
wrote:

Printing IS fixed now...

Ah great! Since the description said it wasn't, I didn't realize the
commits fixing it had gone in too.

But I noticed one problem with printing: if a markdown cell has a
reference to a local image (using the src="files/..." handler we now
have), that image is broken in the print view. I tested and it works
on master, so that problem seems to be specific to this branch...

@ellisonbg
Copy link
Member Author

On Wed, Jan 11, 2012 at 10:52 PM, Fernando Perez
reply@reply.github.com
wrote:

On Wed, Jan 11, 2012 at 10:45 PM, Brian E. Granger
reply@reply.github.com
wrote:

Printing IS fixed now...

Ah great!  Since the description said it wasn't, I didn't realize the
commits fixing it had gone in too.

But I noticed one problem with printing: if a markdown cell has a
reference to a local image (using the src="files/..." handler we now
have), that image is broken in the print view.  I tested and it works
on master, so that problem seems to be specific to this branch...

Can you change that to src="/files/..." to make it absolute? It
should be an absolute path.


Reply to this email directly or view it on GitHub:
#1256 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Copy link
Member

fperez commented Jan 12, 2012

On Wed, Jan 11, 2012 at 11:00 PM, Brian E. Granger
reply@reply.github.com
wrote:

Can you change that to src="/files/..." to make it absolute?  It
should be an absolute path.

Changed, same problem...

@ellisonbg
Copy link
Member Author

Can you double check this. I just made the change in the notebook
tour and the print view comes up fine. But when I print it the page
itself crashes in Chrome :(

On Wed, Jan 11, 2012 at 11:04 PM, Fernando Perez
reply@reply.github.com
wrote:

On Wed, Jan 11, 2012 at 11:00 PM, Brian E. Granger
reply@reply.github.com
wrote:

Can you change that to src="/files/..." to make it absolute?  It
should be an absolute path.

Changed, same problem...


Reply to this email directly or view it on GitHub:
#1256 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg
Copy link
Member Author

@fperez, can you check printing on Linux. It works fine on Mac now - except there is a bug in Chrome's print dialog that is crashing the print view tab in some cases. But Firefox and Safari look great.

@fperez
Copy link
Member

fperez commented Jan 12, 2012

Brian, I'm still seeing problems with a specific notebook. It prints fine with master, so it's something specific to this branch. Go to my tmp directory and download both the ipynb and the svg file. This nb prints fine for me with master on both ffox and chrome (linux), but with this branch, neither browser can print it. Ffox simply ignores the image, and Chrome shows a big 'broken image' area.

@ellisonbg
Copy link
Member Author

@fperez: you still had the files URL relative "files/..." once I made
it absolute "/files/..." it worked fine. Can you retry.

On Thu, Jan 12, 2012 at 3:40 PM, Fernando Perez
reply@reply.github.com
wrote:

Brian, I'm still seeing problems with a specific notebook.  It prints fine with master, so it's something specific to this branch.  Go to my tmp directory and download both the ipynb and the svg file.  This nb prints fine for me with master on both ffox and chrome (linux), but with this branch, neither browser can print it.  Ffox simply ignores the image, and Chrome shows a big 'broken image' area.


Reply to this email directly or view it on GitHub:
#1256 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@fperez
Copy link
Member

fperez commented Jan 12, 2012

On Thu, Jan 12, 2012 at 3:48 PM, Brian E. Granger
reply@reply.github.com
wrote:

@fperez: you still had the files URL relative "files/..." once I made
it absolute "/files/..." it worked fine.  Can you retry.

Argh, I could have sworn I'd changed that last night!!! But I
probably forgot to save the change, and actually made the test from
the unsaved copy.

Yes, it now works fine, from both FFox and Chrome. Sorry for the confusion.

Anything else we need before we can merge this one? It's looking
pretty good to me. Great job!

@ellisonbg
Copy link
Member Author

On Thu, Jan 12, 2012 at 3:56 PM, Fernando Perez
reply@reply.github.com
wrote:

On Thu, Jan 12, 2012 at 3:48 PM, Brian E. Granger
reply@reply.github.com
wrote:

@fperez: you still had the files URL relative "files/..." once I made
it absolute "/files/..." it worked fine.  Can you retry.

Argh, I could have sworn I'd changed that last night!!!  But I
probably forgot to save the change, and actually made the test from
the unsaved copy.

Yes, it now works fine, from both FFox and Chrome.  Sorry for the confusion.

Anything else we need before we can merge this one?  It's looking
pretty good to me.  Great job!

No I think that is it. Thanks for the reviews and testing. I will merge it...


Reply to this email directly or view it on GitHub:
#1256 (comment)

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

@ellisonbg ellisonbg merged commit 88738d0 into ipython:master Jan 13, 2012
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.

None yet

3 participants