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

[notebook] read-only: disable name field #1132

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions IPython/frontend/html/notebook/static/js/notebooklist.js
Expand Up @@ -27,6 +27,8 @@ var IPython = (function (IPython) {


NotebookList.prototype.bind_events = function () {
if (IPython.read_only)
{return}
var that = this;
this.element.bind('dragover', function () {
return false;
Expand Down Expand Up @@ -83,6 +85,9 @@ var IPython = (function (IPython) {
if (!IPython.read_only){
// hide delete buttons when readonly
this.add_delete_button(item);
$('#drag_info').removeClass('hidden');
} else {
$('#drag_info').remove();
Copy link
Member

Choose a reason for hiding this comment

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

On this one, the approach we have gone with elsewhere is to start with class=hidden, and remove that once we know that we are not read-only. That way, we don't flash text on the screen.

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, done, this wasn't the case for surrounding element like new_notebook button, I've changed it also.

}
};
};
Expand Down
4 changes: 4 additions & 0 deletions IPython/frontend/html/notebook/static/js/notebookmain.js
Expand Up @@ -106,6 +106,10 @@ $(document).ready(function () {
$('div#config_section').addClass('hidden');
$('div#kernel_section').addClass('hidden');
$('span#login_widget').removeClass('hidden');

// set the notebook name field as not modifiable
$('#notebook_name').attr('disabled','disabled')
Copy link
Member

Choose a reason for hiding this comment

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

Do you have an aesthetic preference for 'disabled' vs 'readonly'? Functionally identical, but disabled greys out the field. I'm 50-50 on this one.

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 slightly prefer the 'disabled' because it's another visual clue of the read-only mode. I often myself mistaking both mode because you can still select things in RO-mode, especially the blinking cursor also.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, it does serve as a decent indicator of read-only mode (along with the absence of the save button, etc.). We definitely don't want to do something like this to the regular notebook fields, which are just read-only. Those must remain selectable, and should probably not be aesthetically altered. The blinking cursor would maybe be nice to remove, but it should otherwise look unaltered.


// left panel starts collapsed, but the collapse must happen after
// elements start drawing. Don't draw contents of the panel until
// after they are collapsed
Expand Down
Expand Up @@ -33,9 +33,10 @@ $(document).ready(function () {
IPython.login_widget = new IPython.LoginWidget('span#login_widget');

if (IPython.read_only){
$('#new_notebook').addClass('hidden');
// unhide login button if it's relevant
$('span#login_widget').removeClass('hidden');
} else {
$('#new_notebook').removeClass('hidden');
}
IPython.notebook_list.load_list();

Expand Down
Expand Up @@ -20,9 +20,9 @@

{% block content_panel %}
<div id="content_toolbar">
<span id="drag_info">Drag files onto the list to import notebooks.</span>
<span id="drag_info" class="hidden">Drag files onto the list to import notebooks.</span>
<span id="notebooks_buttons">
<button id="new_notebook">New Notebook</button>
<button id="new_notebook" class="hidden">New Notebook</button>
</span>
</div>
<div id="notebook_list">
Expand Down