Skip to content

Conversation

@fcollonval
Copy link
Member

@fcollonval fcollonval commented Sep 7, 2019

fixes #352, fixes #446

This build on #388 but using CodeMirror instead of Monaco. The reason to switch to CodeMirror is to fit better with the visual identity of JupyterLab and nbdime that both use CodeMirror to show code. But the diff are highlighted following a style closer to Monaco than the default CodeMirror addon.

Here is a preview:

image

The remaining tasks:

  • Improve styling to inherit from JupyterLab codemirror styling so changing editor theme is propagate here
  • Add gutter + and -
  • Restructure mergeview.ts
  • Open issues to link codemirror settings changed signal with diff widget to improve integration within jupyterlab

The diff/merge code comes from the merge addon of CodeMirror. This will allow to implement at low cost a plain text merge interface.
The major modification to the original code is the usage of diff-match-patch npm package and the default view for the two panels case is to set the old text on the left panel and the current one in the right panel.

@fcollonval fcollonval marked this pull request as ready for review September 8, 2019 17:15
@fcollonval fcollonval changed the title Plain text diff WIP: Plain text diff Sep 20, 2019
@fcollonval
Copy link
Member Author

@telamonian would you be willing to review this PR?

@telamonian
Copy link
Member

ahh, I see you have another short PR for me, @fcollonval :)

I actually already started going over this PR last weekend, so I'll get through it

Tim Lupo and others added 5 commits December 4, 2019 21:31
First version using CodeMirror

First good looking

Restructure CSS + improve look and feel

Add gutters

Remove `less` call

Use message in lower case

Rebase on master + comment
telamonian
telamonian previously approved these changes Dec 5, 2019
Copy link
Member

@telamonian telamonian left a comment

Choose a reason for hiding this comment

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

A note requesting a future PR, a question, and a couple of minor import sorting niggles. Nothing blocking, so LGTM!

@fcollonval Are you the one who wrote mergeview.ts, or did you get from somewhere else? I'm mostly asking out of curiosity/being impressed. It's supposed to be a Typescript translation of https://github.com/codemirror/CodeMirror/blob/master/addon/merge/merge.js, right?

@fcollonval
Copy link
Member Author

Thanks for the review @telamonian

As you have started creating follow-up issues, I'll merge this one. Feel free to open a new issue, if you think the diff theme for the dark them needs improvements.

Are you the one who wrote mergeview.ts, or did you get from somewhere else? I'm mostly asking out of curiosity/being impressed. It's supposed to be a Typescript translation of https://github.com/codemirror/CodeMirror/blob/master/addon/merge/merge.js, right?

I have not much credit. I copied the original JS file you referenced and modified it to get a Typescript file without too many ts-ignore and without breaking its features.

@fcollonval fcollonval merged commit 9b21bc3 into jupyterlab:master Dec 5, 2019
@fcollonval fcollonval deleted the plaintextdiff-cm branch December 5, 2019 17:48
@damianavila
Copy link
Member

@fcollonval @telamonian this is on master but not released yet, am I right? in general, Is the milestone associated with the PRs/issues a reliable way to follow the status quo? Should I look into other documents besides the roadmap? Thanks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Failed to fetch diff Default diff viewer

4 participants