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

Tabs #2206

Closed
wants to merge 7 commits into from
Closed

Tabs #2206

wants to merge 7 commits into from

Conversation

dlsun
Copy link

@dlsun dlsun commented Jul 26, 2012

Working for the most part. Some known issues:

  • Issues with tabs for brand new notebooks. Try adding a tab to a new notebook to see what I mean. The problems go away if you refresh the page just once.
  • The user is allowed rename a tab to blank. I tried to prevent this behavior by adding a blur event to the textbox (see tabmanager.js), but that event handler is circumvented if you click on the textbox more than once.

@@ -0,0 +1,151 @@
#!python
Copy link
Member

Choose a reason for hiding this comment

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

This file got here by accident, it's not supposed to be here. In fact, the whole scripts/ directory under static/ shouldn't be there at all.

@fperez
Copy link
Member

fperez commented Jul 26, 2012

Hi @dlsun, this is great to see! This PR will probably need some history cleanup before merging, because unfortunately the current history is a bit of a mess and it touches files that shouldn't have been modified in the first place. But don't worry: we'll be happy to give you a hand with that, the first priority is to get the (extremely important) tab functionality working robustly, we're very excited to see progress on this front.

We'll be playing with this over the next few days and providing you with more specific feedback. Thanks for contributing!

@dlsun
Copy link
Author

dlsun commented Jul 26, 2012

Yes, I'm very sorry about the misplaced files and the messy history -- I
had a couple of git mishaps along the way. (I essentially created a git
repository inside a subdirectory of main git repository without realizing
it.)

Please let me know what I need to do to clean the git history; it's
something I would like to learn!

@fperez
Copy link
Member

fperez commented Jul 26, 2012

On Wed, Jul 25, 2012 at 11:47 PM, dlsun
reply@reply.github.com
wrote:

Yes, I'm very sorry about the misplaced files and the messy history -- I
had a couple of git mishaps along the way. (I essentially created a git
repository inside a subdirectory of main git repository without realizing
it.)

Please let me know what I need to do to clean the git history; it's
something I would like to learn!

You'll want to familiarize yourself with 'git rebase'. The tutorials
at github are in general pretty good, and I have some additional links
to git resources here: http://fperez.org/py4science/git.html

You'll want to rebase this branch, removing the commits that are
irrelevant and leaving no merges from the main repo, so that we can
evaluate only your new changes in isolation from anything else that
happened in the project.

You can make a 'backup' branch easily by creating a new branch off
this one in your local repo before starting the rebase process. That
way if you make a mistake, it's trivial to find again the point where
you started (it's always possible to do so, as git doesn't destroy
your data, but having a named backup makes it easiear and less scary).

@Carreau
Copy link
Member

Carreau commented Jul 26, 2012

looks cool.
2 comments.

Please indent with space to be consistent with the rest of the code
Issues with new notebooks are due to 'document ready' called twice. No idea why.

I'll look at the code later.

});
this.element.find('#insert_worksheet_below').click(function() {
IPython.notebook.insert_worksheet_below('New Sheet');
});
this.element.find('#insert_cell_above').click(function () {
Copy link
Member

Choose a reason for hiding this comment

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

I would change bellow/above for left/right, what do you think ?

@Carreau
Copy link
Member

Carreau commented Jul 26, 2012

That's a lot a code... I didn't read it all yet.
The selection of element in the DOM seem sometime complicated, but I need to understand it better first.
I'm not event sure about all the comment I made.
I read it on github and it seem that it use 2spaces tabs, so it's hard to read.

@dlsun
Copy link
Author

dlsun commented Jul 26, 2012

Thanks for all the suggestions.

@Carreau, I like many of your Javascript suggestions, although most of the functions to handle the worksheets are adapted versions of existing functions that were handling the cells, so if we change the Javascript for the worksheets we should also change it for the cells, for consistency.

I agree that getting the worksheets is a major pain. The problem is that when you reorder the tabs, the div elements corresponding to the panels don't get reordered, so when you save the notebook, it saves the worksheets in the original order. The messy code that I have is just to get the ordering of the tabs, and then fetch each div element in that order. If anyone has a better way of doing this, I'd be interested to know.

I'll rebase tonight.

@Carreau
Copy link
Member

Carreau commented Jul 26, 2012

@Carreau, I like many of your Javascript suggestions, although most of the functions to handle the worksheets are adapted versions of existing functions that were handling the cells, so if we change the Javascript for the worksheets we should also change it for the cells, for consistency.

Sure, but it would be better to do it separately, this PR is big enough.

I agree that getting the worksheets is a major pain. The problem is that when you reorder the tabs, the div elements corresponding to the panels don't get reordered, so when you save the notebook, it saves the worksheets in the original order. The messy code that I have is just to get the ordering of the tabs, and then fetch each div element in that order. If anyone has a better way of doing this, I'd be interested to know.

Oh? you can drag to reorder ? I'll try tomorrow.

@Carreau
Copy link
Member

Carreau commented Jul 28, 2012

Did you had a change to rebase and convert the tabs to spaces ?

@dlsun
Copy link
Author

dlsun commented Jul 29, 2012

@Carreau, I've cleaned up the history (I ended up starting from a clean copy of master and replacing only the files that needed to be changed), and replaced tabs by spaces.

@Carreau
Copy link
Member

Carreau commented Jul 29, 2012

hi,
Thanks for the rebase, there is still a small conflict thought.
Otherwise this is broken for me see capture :
Imgur
I have no way to scroll the menubar back visible, and the pager is below the cells.

Also imho, tabs take a lot a space and I would be greate to have a way to hide them and/or make them scroll with the notebook and be hidden below the menubar. Not sure about this one.

@@ -50,8 +50,10 @@ var IPython = (function (IPython) {
$('div#pager').height(pager_height);
if (IPython.pager.expanded) {
$('div#notebook').height(app_height-pager_height-pager_splitter_height);
$('div.panel').height(app_height-pager_height-pager_splitter_height);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry i'll nitpick...
wrong indent level.

@Carreau
Copy link
Member

Carreau commented Jul 29, 2012

the problem at first load is due to this
I think it's here only for new notebook i'll open a issue to change this to a real redirect.


$([IPython.events]).on('set_dirty.Worksheet', function (event, data) {
that.dirty = data.value;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when there are multiple worksheets? Triggering set_dirty.Worksheet sets the dirty flag for all worksheets?

Same question for set_next_input.Worksheet. What happens if data.cell is in another worksheet?

@tkf
Copy link
Contributor

tkf commented Aug 19, 2012

Just curious. Why IPython.events uses method_like_name.ClassName? Is it some jQuery specific stuff? I always wonder why it does not use ClassName.method_like_name.

@Carreau
Copy link
Member

Carreau commented Sep 28, 2012

Trying to revive old PRs.
Any progress on this one ?

@ellisonbg
Copy link
Member

I haven't dived into the code yet, but have some general comments:

  • We are wanting to quickly move to using bootstrap, rather than jquery UI. @Carreau do you think we should consider eliminating all jQuery UI usage, or should we make the transition more slowly? I am wondering if we want to implement the tab UI using bootstrap from the start.
  • We will need a complex set of capabilities for the tab UI: renaming, new, moving, removing, cut/copy/paste between worksheets, etc. I am not excited about adding lots of visual complexity to our UI to implement this. What about a simple CLI for manipulating notebook documents. Something like `movecell(1.1,2.3)' = move cell 1 ws 1 to below cell 3 ws 2 instead? We could do a lot of powerful things with something like that as well.
  • Tabs are a great solution when there are only a few of them. Whern there are more it gets tough. Can we think of a tabbing UI that scales better?

@Carreau
Copy link
Member

Carreau commented Oct 4, 2012

I haven't dived into the code yet, but have some general comments:

We are wanting to quickly move to using bootstrap, rather than jquery UI. @Carreau do you think we should consider eliminating all jQuery UI usage, or should we make the transition more slowly? I am wondering if we want to implement the tab UI using bootstrap from the start.

I would go slowly.
To start, we can rename our jqueryUI.css to a .less file and include it in bootstrap so that it compiles with less as mixins and get rid of it progressively.

We will need a complex set of capabilities for the tab UI: renaming, new, moving, removing, cut/copy/paste between worksheets, etc. I am not excited about adding lots of visual complexity to our UI to implement this. What about a simple CLI for manipulating notebook documents. Something like `movecell(1.1,2.3)' = move cell 1 ws 1 to below cell 3 ws 2 instead? We could do a lot of powerful things with something like that as well.

+1 If I understand correctly custom toolbar wil allow people to customize.

Tabs are a great solution when there are only a few of them. Whern there are more it gets tough. Can we think of a tabbing UI that scales better?

Some things like tabs have to be visible, but can be smaller.
Why not a shortcut to hide all UI but toolbar/ restore UI ?

@ellisonbg
Copy link
Member

This is important work that has been sitting for a while, but it needs to be rebased. It also needs further review. @dlsun can re rebase this so review can continue?

@dlsun
Copy link
Author

dlsun commented Dec 5, 2012

I would love to see this worked into the codebase, but unfortunately I no
longer have time to work on it. Perhaps someone could take over (or even
start from scratch -- I didn't change too much code).

On Tue, Dec 4, 2012 at 2:26 PM, Brian E. Granger
notifications@github.comwrote:

This is important work that has been sitting for a while, but it needs to
be rebased. It also needs further review. @dlsunhttps://github.com/dlsuncan re rebase this so review can continue?


Reply to this email directly or view it on GitHubhttps://github.com//pull/2206#issuecomment-11018592.

@ellisonbg
Copy link
Member

OK I am going to close this. We are using issue #1905 to track progress on this feature and have linked to this PR for future work.

@Carreau
Copy link
Member

Carreau commented Feb 10, 2013

Do not continue work on this before speeking to the core developers.
Worksheet data structure might be removed.

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

5 participants