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

[toc2] update for cells width ans sidebar height #994

Merged
merged 5 commits into from
May 25, 2017

Conversation

jfbercher
Copy link
Member

@jfbercher jfbercher commented May 20, 2017

This address issue #993. In case nothing is selected (no widenNotebook and no Sidebar), then don't modify or restore cells width to the original width.
In passing I see that I had a pending modification that should improve the transition effect when toggling the sidebar visibility.
Also recompute sidebar height on window resize event.

@@ -176,15 +177,15 @@ var make_link = function(h, num_lbl) {
$('#notebook-container').css('margin-left', 30);
$('#notebook-container').css('width', $('#notebook').width() - 30);
} else { // original width
$("#notebook-container").css({'width':"82%", 'margin-left':'auto'})
$("#notebook-container").css({'width':st.nbcontainer_width, 'margin-left':'auto'})
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 this should probably set an empty string, to not set anything, otherwise it'll break when the window is resized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right. thanks

Copy link
Member

@jcb91 jcb91 left a comment

Choose a reason for hiding this comment

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

Mainly looks ok, but I think I'm missing something to do with the new parameter for setNotebookWidth

//cfg.widenNotebook = false;
function setNotebookWidth(cfg, st, sideBarIsToggling) {
//cfg.widenNotebook = true;
var sideBarIsToggling = typeof sideBarIsToggling !== 'undefined' ? sideBarIsToggling : false;
Copy link
Member

Choose a reason for hiding this comment

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

this sideBarIsToggling variable doesn't actually seem to get used anywhere - what is it intended for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing. Indeed it is a dead code from a previous attempt. I remove it.

@@ -176,19 +177,28 @@ var make_link = function(h, num_lbl) {
$('#notebook-container').css('margin-left', 30);
$('#notebook-container').css('width', $('#notebook').width() - 30);
} else { // original width
$("#notebook-container").css({'width':"82%", 'margin-left':'auto'})
$("#notebook-container").css({'width':'', 'margin-left':'auto'})
Copy link
Member

Choose a reason for hiding this comment

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

the margin-left should probably also be reset here (admittedly, it does end up as auto with the default css, but it's neater to just remove the override, in case anyone is trying to customize things)

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@jfbercher jfbercher changed the title [toc2] update for sidebar toggling and cells width [toc2] update for cells width ans sidebar height May 21, 2017
@jfbercher
Copy link
Member Author

Thanks to @jcb91 for reviewing and directions for improvements.

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.

2 participants