-
Notifications
You must be signed in to change notification settings - Fork 4
Add a "View Only" header for shared notebooks (do not merge) #72
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
Add a "View Only" header for shared notebooks (do not merge) #72
Conversation
|
The 400+ line diff here at the time of writing is primarily due to the extent to which I had to go in order to make the styling work and look as closely as possible to the Figma design documents we have. |
|
It would be helpful if we had this tested on CI. |
| } | ||
|
|
||
| /** | ||
| * Checks if a notebook is read-only/shared based on its metadata. |
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.
This is a tight coupling to the backend. Instead I would suggest to make sure we manage the writable state which is used natively by JupyterLab (which we already do) and then use this one-liner:
return !notebookPanel.context.contentsModel.writable| if (!contentHeader) { | ||
| console.error('NotebookPanel.contentHeader is not available'); | ||
| return null; | ||
| } |
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.
Why would it be unavailable? This seems a bit too defensive.
| * @param notebookPanel - The notebook panel to style | ||
| * @param headerWidget - The View Only header widget | ||
| */ | ||
| function applySeamlessHeaderStyling(notebookPanel: any, headerWidget: Widget): void { |
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.
Why can't we just use CSS for this?
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.
I think we could. I very much do not undersand what this function was intended to do.
In any case I opened #75 as a counter proposal.
2a98d24 to
96f5686
Compare
|
bot please update snapshots |
|
This did not work the last time because none of the changes in this PR gets applied immediately. I added a delay and will regenerate snapshot: bot please update snapshots |
|
Superseded by #75. |
Tip
This PR is currently not in its final state, but it is ready for review.
At this moment, the styling and the
View Onlyheader are working, but I have yet to separate the implementation into a new file, remove redundant styling, and find a way to prevent removing theread-only-indicatorfrom the toolbar of widgets, among other tasks.