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

Implement viewport tracking and reactive rendering in scrollbar #16392

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

krassowski
Copy link
Member

References

Code changes

  • WindowedList.IRenderer.createScrollbarItem can now return either HTMLElement (as before) or IScrollbarItem defined as:
    interface IScrollbarItem extends IDisposable {
      /**
       * Render the scrollbar item as an HTML element.
       */
      render: (props: { index: number }) => HTMLElement;
    
      /**
       * Unique item key used for caching.
       */
      key: string;
    }
  • The entire scrollbar is no longer re-rendered on each update of the notebook; this enables adding more reactive information and updating only the cells that have changed; instead IScrollbarItem.render is called to modify the node reactively
  • a new viewport indicator element is added which shows which cells are visible in the viewport (styling subject to change); this element is independent of cells to avoid scrolling causing re-render of items

User-facing changes

viewport-indicator

Backwards-incompatible changes

None

@krassowski krassowski added this to the 4.3.0 milestone May 29, 2024
Copy link

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

@github-actions github-actions bot added pkg:notebook pkg:ui-components tag:CSS For general CSS related issues and pecadilloes Design System CSS labels May 29, 2024
@krassowski krassowski marked this pull request as ready for review May 29, 2024 17:00
@srdas
Copy link

srdas commented May 29, 2024

@krassowski - This is really nice! As a parallel, I use the mini view in VSCode to traverse quickly to where my edits to the source file are, and I am not sure if this is a useful way to use the viewport in the notebook. If so, adding color to modified cells may be useful. It would definitely be nice to add this and get some feature parity with other IDEs.

@krassowski
Copy link
Member Author

Thank you for the suggestion! I implemented it in #16432.

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 @krassowski, this looks great.

There is currently a gap between a selected item and the top separator, but not the bottom separator. And the viewport also seems to be a bit too low

With changes proposed below, it seems more regular:

packages/ui-components/src/components/windowedlist.ts Outdated Show resolved Hide resolved
packages/ui-components/style/windowedlist.css Outdated Show resolved Hide resolved
@brichet
Copy link
Contributor

brichet commented Jun 21, 2024

Sorry I split the review for a better readability.

For a long notebook, the virtual scrollbar does not fit on the page, and the scrollbar viewport disappears.

Do you think the virtual scrollbar could be scrolled, to follow the scrollbar viewport ?

@krassowski
Copy link
Member Author

Do you think the virtual scrollbar could be scrolled, to follow the scrollbar viewport ?

Yes, I had the same thought and completely agree that this could be useful, but since this is an additional behaviour change (as of today the virtual scrollbar does not scroll when you move the viewport outside of it) I would defer it to a separate PR. I opened #16518 to track this.

krassowski and others added 2 commits June 21, 2024 09:54
Co-authored-by: Nicolas Brichet <32258950+brichet@users.noreply.github.com>
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 @krassowski, looks good to me.

@krassowski krassowski merged commit f364345 into jupyterlab:main Jun 21, 2024
83 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants