-
Notifications
You must be signed in to change notification settings - Fork 4
Implement view-only notebook using custom factory #75
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
Implement view-only notebook using custom factory #75
Conversation
|
My plan would be:
|
|
bot please update snapshots |
ba55b54 to
d3251ed
Compare
|
bot please update snapshots (again, again, one last time) |
e1d219a to
7fe19d4
Compare
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @krassowski! Your implementation is much better than mine 😄
Not for this PR, but is there a way to remove the "Click to add a cell" container below the cell and the four buttons inside the cell, as they can be used to change the order of the cells/add or delete a cell/etc. (not that those changes will be persisted to CKHub though, but maybe it makes sense to not allow them in the UI if it's a view-only notebook):
| "jupyter.lab.transform": true, | ||
| "properties": { | ||
| "toolbar": { | ||
| "title": "View-only notebook panel toolbar items", | ||
| "type": "array", | ||
| "default": [] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: does this remove the "notebook is read-only" bar as well, as you're not manipulating the DOM to do so and instead adding back the buttons for ViewOnlyNotebook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that item gets removed too, but in a different way, here:
jupyterlite-extension/src/view-only.ts
Lines 74 to 79 in 7fe19d4
| super({ | |
| ...options, | |
| toolbar: new FilteredToolbar({ | |
| itemsToFilterOut: new Set(['read-only-indicator']) | |
| }) | |
| }); |
jupyterlite-extension/src/view-only.ts
Lines 50 to 63 in 7fe19d4
| class FilteredToolbar extends ReactiveToolbar { | |
| constructor(options: FilteredToolbar.IOptions) { | |
| super(options); | |
| this._itemsToFilterOut = options.itemsToFilterOut; | |
| } | |
| insertItem(index: number, name: string, widget: Widget): boolean { | |
| if (this._itemsToFilterOut?.has(name)) { | |
| return false; | |
| } | |
| return super.insertItem(index, name, widget); | |
| } | |
| // This can be undefined during the super() call in constructor | |
| private _itemsToFilterOut: Set<string> | undefined; | |
| } |
| // Even though we have a custom view-only factory, we still | ||
| // want to indicate that notebook is read-only to avoid | ||
| // error on Ctrl + S and instead get a nice notification that | ||
| // the notebook cannot be saved unless using save-as. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One follow-up here after this PR could be to style that notification as well. The default design is pretty neat, so maybe all we need to do is to change the orange-ish colour to something like --je-slate-blue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we could adjust the styling. Maybe we could track that in a follow-up issue - would you like to open one?
Though orange/yellow is meant to symbolize danger so maybe not a bad choice? But surely there is some rounding and borders we could adjust.
@agriyakhetarpal this is no longer the case with this PR, see: |

View Onlydiv using theContentsHeader#35ViewOnlyNotebooktoolbar