-
Notifications
You must be signed in to change notification settings - Fork 28.4k
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
Interactive window- don't leave space for insert toolbar #181949
Conversation
@amunger I have cleaned up my previous PR you reviewed a long while ago. Could you take a look? |
@@ -145,7 +145,7 @@ export class NotebookOptions extends Disposable { | |||
const cellToolbarInteraction = overrides?.cellToolbarInteraction ?? this.configurationService.getValue<string>(NotebookSetting.cellToolbarVisibility); | |||
const compactView = this.configurationService.getValue<boolean | undefined>(NotebookSetting.compactView) ?? true; | |||
const focusIndicator = this._computeFocusIndicatorOption(); | |||
const insertToolbarPosition = this._computeInsertToolbarPositionOption(); | |||
const insertToolbarBetweenCells = overrides?.insertToolbarBetweenCells ?? this._computeInsertToolbarBetweenCellsOption(); |
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.
should we check if the notebook document is readonly or not?
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.
Yep, it is a bigger change now but it looks great. I used Show Contents
on local history timeline/git timeline to see a readonly jupyter notebook.
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.
insertToolbarBetweenCells
should be set to false here if the notebook is readonly. That would eliminate the need to pass around the readonly parameter in all those other methods.
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.
That information comes from the model, NotebookEditorWidget
doesn't have the model in its constructor, only later when NotebookEditorWidget.setModel
is called (which can be done multiple times)
Here's the call stack when using Show Contents on a git revision (creationOptions
is { menuIds: {... }, cellEditorContributions: [ ...] }
)
Then setOptions with isReadOnly set-
bottomToolbarGap: 0, | ||
bottomToolbarHeight: 0 | ||
} : { | ||
bottomToolbarGap: 12, |
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 enough gap for the cellToolbar
to have a gap between the top of the toolbar and the editor outline of the previous cell (in the case the previous cell has no output)
17434cd
to
761a1d3
Compare
Interactive window is more compact than Notebook now, by not leaving space for the insert cell toolbar(which has no items anyway in the interactive window)
This is cleaned up from #164760