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

Use Monaco for displaying diffs (fixes #2275, #2260) #2334

Merged

Conversation

mehmetoguzderin
Copy link
Contributor

Aims to solve #2275 and #2260.

This pull request migrates generation of content for revision-diff from ReactDiffViewer to DiffEditor provided by monaco itself.

revision-diff-content serves as a non-redux wrapper for abstracting details of monaco away. Further connections such as theming should be communicated as properties from revision-diff to revision-diff-content in render function.

@wlach
Copy link
Contributor

wlach commented Oct 29, 2019

CI for this is failing because jest can't handle Monaco's esm syntax (see react-monaco-editor/react-monaco-editor/#176). A simple fix is to use identity-object-proxy, since we don't actually care about the behaviour of the editor component for the purposes of testing our own business logic.

@wlach wlach force-pushed the 2019-10-revision-diff-content branch from 11575f2 to 7d4370c Compare October 29, 2019 15:11
@codecov
Copy link

codecov bot commented Oct 29, 2019

Codecov Report

Merging #2334 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2334   +/-   ##
=======================================
  Coverage   69.51%   69.51%           
=======================================
  Files         223      223           
  Lines        5451     5451           
  Branches      877      877           
=======================================
  Hits         3789     3789           
  Misses       1643     1643           
  Partials       19       19
Impacted Files Coverage Δ
src/editor/components/modals/revision-diff.jsx 48.78% <ø> (ø) ⬆️

@wlach
Copy link
Contributor

wlach commented Oct 29, 2019

To expedite getting this (lovely lovely) feature landed, I fixed up the issue above locally and pushed to @mehmetoguzderin's branch. Assuming this works, I'll go ahead and merge. Thank you Oguz for doing the lion's share of the work here!

@wlach wlach changed the title Use DiffEditor of monaco (fix for #2275 and #2260) Use Monaco for displaying diffs (fixes #2275, #2260) Oct 29, 2019
@wlach wlach force-pushed the 2019-10-revision-diff-content branch from 7d4370c to cdb9db8 Compare October 29, 2019 15:34
mehmetoguzderin and others added 3 commits October 29, 2019 11:35
We don't actually need to import monaco to test the business
logic of the diff viewer, and jest doesn't understand the ESM syntax
it uses (see e.g. react-monaco-editor/react-monaco-editor/iodide-project#176).
@wlach wlach force-pushed the 2019-10-revision-diff-content branch from cdb9db8 to eab03a5 Compare October 29, 2019 15:35
@wlach wlach merged commit fe7b512 into iodide-project:master Oct 29, 2019
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