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 minimap in the virtual scrollbar #16432

Merged
merged 4 commits into from
Jun 28, 2024

Conversation

krassowski
Copy link
Member

@krassowski krassowski commented Jun 4, 2024

References

For now this PR includes two PRs on which it depends:

Code changes

Creates a private (non-exported) ScrollbarItem class which implements reactive scrollbar item allowing for high-performance rendering of minimap even in very large notebooks.

User-facing changes

The virtual scrollbar now exposes information on whether the cell:

  • is running/scheduled ([*]), idle ([ ]) or was already run (e.g. [1])
  • was modified since it was last executed ("dirty") - orange background - thank you @srdas for the suggestion
  • has output contains an error (traceback) - red background
  • is markdown/code ([X] for code, nothing for markdown)
  • roughly what text content does it contain

wip

running-and-modified

Plan

Depending on feedback:

  • make the display of source code configurable?
    • allow to configure maximum number of lines or height shown?
  • make the entire thing configurable, allowing fallback to previous simpler implementation?

Ideas for future improvements:

  • option for viewport to be always visible
  • option to show headings as a line with bold?

Backwards-incompatible changes

None

@krassowski krassowski added this to the 4.3.0 milestone Jun 4, 2024
Copy link

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link: Binder

@ellisonbg
Copy link
Contributor

Ohh, very cool! Some minor feedback:

  • I think the rendering of the prompt/cell number using [1] in the minimap cells works well. However, we might want to try to display the prompt at a slightly smaller size. It is a bit hard to tell just from a screenshot, but it looks like the same size as the actual prompt. Maybe one or two stops smaller?
  • I realize the value of putting the icon button right above the UX. However the kernel name and kernel status indicator have been in that position for a long time (since the beginning). Moving them to the right could confuse users who have grown used to their location (those items are all related to the kernel and its status). Could you try putting it to the right of the cell type selector as a variation? Another downside of its current location is that it would be the first icon to disappear (into the ... button) when the toolbar gets narrow.

If it isn't too much trouble, could you generate some alternate screenshots to help us explore and weight the different visual options.

@krassowski
Copy link
Member Author

krassowski commented Jun 11, 2024

Thank you for leaving feedback @ellisonbg!

I think the rendering of the prompt/cell number using [1] in the minimap cells works well. However, we might want to try to display the prompt at a slightly smaller size. It is a bit hard to tell just from a screenshot, but it looks like the same size as the actual prompt. Maybe one or two stops smaller?

I agree that it should be smaller than the actual prompt. There is of course a limit as to how small we can make it to keep it legible. Currently the prompt in the notebook cells are 13px and in the the minimap they are 80% of that which works out to 10.4px. Visually they may seem more similar because the former are grey and the latter are black (left - minimap as of now, right - cell in a notebook)

image

I realize the value of putting the icon button right above the UX. However the kernel name and kernel status indicator have been in that position for a long time (since the beginning). Moving them to the right could confuse users who have grown used to their location (those items are all related to the kernel and its status). Could you try putting it to the right of the cell type selector as a variation? Another downside of its current location is that it would be the first icon to disappear (into the ... button) when the toolbar gets narrow.

To clarify, this PR is not adding the hamburger icon button to the toolbar. For context, that button (and the initial virtual scrollbar implementation) was added 6 months ago in #15533 by @afshin after previous discussion in #15109 (and discussing this on multiple JupyterLab calls). Discussing it in a dedicated issue rather than here would give it more prominence and allow users who only monitor the issues by screening the titles to participate so I opened #16478 for it. Also, if we were to make this change here, this PR would grow to touch dozens of snapshot files and toolbar tests, so I would really prefer not to do that.

@krassowski krassowski marked this pull request as ready for review June 12, 2024 16:10
@krassowski krassowski mentioned this pull request Jun 19, 2024
9 tasks
@andrewfulton9
Copy link
Contributor

I tested this and it works well for me (Ubuntu 20.04.6 LTS, firefox 114.0.2). Scrolling works well, execution indicator, ect. all work smoothly and as expected on the examples/OutputExamples.ipynb notebook.

@brichet
Copy link
Contributor

brichet commented Jun 24, 2024

Thanks @krassowski for adding this feature, it's really nice.

In my opinion, we should make it configurable, with this display as default.

  • make the display of source code configurable?

    • allow to configure maximum number of lines or height shown?

We could have an option to fully render the cells, which would remove the max-height of .jp-scrollbarItem-source and the limitation to 10000 characters.

  • make the entire thing configurable, allowing fallback to previous simpler implementation?

In the case of a simple rendering (rectangle with cell index), would it be easy (and necessary) to fallback to a simple HTMLElement, as it was before #16392, to avoid any computation ?

@brichet
Copy link
Contributor

brichet commented Jun 24, 2024

Is this expected that there is no new line in markdown cells ?

image

@krassowski
Copy link
Member Author

Is this expected that there is no new line in markdown cells ?

I guess you did not expect it, so probably not exactly.

The current behaviour corresponds to white-space-collapse: collapse. If we added white-space: pre as in code cells then it would not accurately represent long lines in markdown. So neither is great. However, we can use white-space: pre-line (a.k.a white-space-collapse: preserve-breaks) which seems like a good option to me.

Top - markdown code, bottom - rendered:

default (current) white-space: pre preserve-breaks
image image image

@krassowski
Copy link
Member Author

In my opinion, we should make it configurable, with this display as default.

I agree. My original intent was to add settings in this PR but I realised that we will probably want the minimap to be an extension point and therefore we should not be adding settings to the Notebook package because then it would be a breaking change to remove them (which would be needed to move then to a new plugin providing the default implementation so that extensions can provide their own settings schema), so let's settle on the extension API for minimap first and implement settings second. I opened #16528 to track this.

@krassowski
Copy link
Member Author

In the case of a simple rendering (rectangle with cell index), would it be easy (and necessary) to fallback to a simple HTMLElement, as it was before #16392, to avoid any computation ?

This could be implemented as a separate plugin (disabled by default) which could be swapped by the user if they wanted the other implementation once we implement an extension point for the minimap (#16528); that said I am not sure how much value there would be in it.

Copy link
Contributor

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks for the changes and details @krassowski, this looks good.

Should we add #16528 in 4.3 release plan ?

@krassowski
Copy link
Member Author

Thank you for the review!

Should we add #16528 in #16315 ?

Done!

@krassowski krassowski merged commit 1d3e4ac into jupyterlab:main Jun 28, 2024
84 checks passed
afshin pushed a commit to afshin/jupyterlab that referenced this pull request Jul 1, 2024
* Notebook minimap in the virtual scrollbar

* Remove `updateOutput` in favour of making `updatePrompt` an arrow func

* Improve line breaks behaviour in markdown cell previews

* Adjust prompt positioning after c26de19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design System CSS feature pkg:notebook tag:CSS For general CSS related issues and pecadilloes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encoding more information in the virtual scroll bar "minimap" of notebook structure for navigation
4 participants