-
Notifications
You must be signed in to change notification settings - Fork 3
Unified cell and file diffs #16
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
Conversation
|
|
||
| commands.addCommand('jupyterlab-cell-diff:show-codemirror', { | ||
| label: trans.__('Show Cell Diff (CodeMirror)'), | ||
| commands.addCommand('jupyterlab-cell-diff:split-cell-diff', { |
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 changes the id of the existing command, so it's aligned with the names of the new commands.
|
Love this PR! The unified diff view is a great addition. 🎉 I have some minor UX suggestions that are totally up for discussion - these are pretty nitpicky and Icon ChoicesAccept All: Reject All:
The undo icon is actually quite nice here because it's:
That said, it's worth considering if users might expect a more traditional "discard" icon. Curious Button UX and stylingThe button spacing could use some minor tweaks:
Here are some values to try and see if the spacing feels better: .jp-UnifiedDiff-acceptAll,
.jp-UnifiedDiff-rejectAll,
.jp-UnifiedFileDiff-acceptAll,
.jp-UnifiedFileDiff-rejectAll {
gap: 6px;
}
.jp-UnifiedDiff-rejectAll,
.jp-UnifiedFileDiff-rejectAll {
margin-right: 8px;
}It might be helpful to add subtle color cues to differentiate Accept vs Reject actions (while keeping it accessible) Auto-dismiss the cell footer instead of "X" iconThe unified diff has a different UX pattern than the dropdown split-cell diff:
Because of this difference, the X button in the cell footer isn't needed here - the buttons should Again, these are just suggestions for future polish. The PR is great as-is! 🚀 |
|
Also, a question: what happens if the user runs the cell while the diff is still showing? Does it run the old source or new source? |
Using the new content makes the most sense to me. |
Sure, totally fine. |
|
This PR adds a couple of basic UI tests to cover the three commands. There are all quite similar for now, with one test file per command. We should likely add more, for example to:
But happy to stop here for now in case folks are already interested in using the new commands. But at least we have the basic structure to add more tests now. |
|
I'm comfortable merging without further tests and doing this work in follow up PRs. I'd love to start using this work right away. 😎 |
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.
Looks great, @jtpio! Thank you for your amazing work here.

Fixes #3
Fixes #10
The demos below were made with the agent from https://github.com/jupyterlite/ai, which uses the commands defined in
jupyterlab-cell-diffto show the diffs:Unified cell diffs
jupyterlab-cell-diff-unified-cell-diff.mp4
Unified file diffs
jupyterlab-cell-diff-file-diffs.mp4