Skip to content

Conversation

@gwincr11
Copy link
Contributor

@gwincr11 gwincr11 commented Oct 24, 2022

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:

  • 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.

Screen Shot 2022-10-24 at 12 33 59 PM

@gwincr11
Copy link
Contributor Author

gwincr11 commented Nov 8, 2022

@vidartf any hints on how to get the SVG to render properly? If not I can inline it 😅

@vidartf
Copy link
Collaborator

vidartf commented Nov 22, 2022

@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)
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@gwincr11
Copy link
Contributor Author

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.
@gwincr11 gwincr11 force-pushed the cg-refactor-widget branch from 8f19587 to 3f9c36b Compare March 13, 2023 14:37
Copy link
Collaborator

@vidartf vidartf left a 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> No newline at end of file
Copy link
Collaborator

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-outer-wrapper: rgb(221, 244, 255, 1);
--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);
Copy link
Collaborator

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?

@vidartf
Copy link
Collaborator

vidartf commented Nov 10, 2023

I opened a rebased version of this PR in #733.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants