-
Notifications
You must be signed in to change notification settings - Fork 160
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
Make Cells Display Lazily When Diffing Unchanged Cells #636
base: master
Are you sure you want to change the base?
Conversation
21b6cd8
to
f56db7c
Compare
@vidartf any hints on how to get the SVG to render properly? If not I can inline it 😅 |
I'm unsure what the best approach is here. I think if we want import to work, it will be a breaking change, as any library that depends on nbdime will now have to figure out bundling rules for SVGs. I think also some care need to be taken about adding classes etc to the SVG elements, so that JupyterLab can theme it properly. I think it might be good to read up on https://jupyterlab.readthedocs.io/en/stable/extension/ui_components.html#how-to-create-your-own-custom-labicon |
--jp-expand-outer-wrapper: rgb(221, 244, 255); | ||
--jp-expand-color: rgb(87, 96, 106); | ||
--jp-expand-a-color: rgba(84, 174, 255, 0.4) |
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.
Nitpick: Ideally concepts of the DOM should not bleed into the CSS variable names. Ideally it would say something about the intended difference/relationship between the different variables, so that it can more easily by themed by others (and have those themes survive a change to the DOM).
Also, this color has an alpha element in order to work on multiple themed backgrounds. The other colors in these variables should ideally also work in the same constraints. Mainly since default JupyterLab themes do not include nbdime theming, but we still want nbdime to work with all of them.
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.
Ok I changed these to not reflect the dom elements that use them.
I seem a bit stuck on this since typescript stoped compiling. I opened a pr to update typescript which will allow me to complete this pr, thanks |
Motivation: During diffing Unchanged cells are displayed, which is not necessary. This is a performance issue when there are many unchanged cells. Related Issues: - jupyter#635 Open Questions: - I am struggling with getting the svg images to build properly. - How can this be tested in JupyterLab? Changes: - Alter Notebooks Widget to only display changed cells by using new linked list of cells. - Add new linked list of cells and lazy version of linked list of cells.
8f19587
to
3f9c36b
Compare
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.
Just doing a quick code-diff only review (still need to pull and build for manual checks):
@@ -0,0 +1 @@ | |||
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="24" height="24"><path fill-rule="evenodd" d="M12 19a.75.75 0 01-.53-.22l-3.25-3.25a.75.75 0 111.06-1.06L12 17.19l2.72-2.72a.75.75 0 111.06 1.06l-3.25 3.25A.75.75 0 0112 19z"></path><path fill-rule="evenodd" d="M12 18a.75.75 0 01-.75-.75v-7.5a.75.75 0 011.5 0v7.5A.75.75 0 0112 18zM10.75 6a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1a.75.75 0 01-.75-.75zm-8 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1A.75.75 0 012.75 6zm12 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1a.75.75 0 01-.75-.75zm-8 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1A.75.75 0 016.75 6zm12 0a.75.75 0 01.75-.75h1a.75.75 0 010 1.5h-1a.75.75 0 01-.75-.75z"></path></svg> |
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.
Does these icons style in lab when the theme changes? (not able to do a build rn) If not, please check these docs: https://jupyterlab.readthedocs.io/en/stable/extension/ui_components.html#sync-icon-color-to-jupyterlab-theme
--jp-expand-color: rgb(87, 96, 106, 1); | ||
--jp-expand-activate-color: rgba(84, 174, 255, 0.4); | ||
--jp-expand-activate-focus-color: rgb(14, 94, 208, 1); |
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.
Could these be re-written to be partially transparent, so that they have a fighting chance of working on both dark an light themes? Alternatively be based on JupyterLab CSS variables so that they might have inherent styling?
I opened a rebased version of this PR in #733. |
Motivation:
During diffing Unchanged cells are displayed, which is not necessary.
This is a performance issue when there are many unchanged cells.
Related Issues:
Open Questions:
Changes: