Skip to content

import diff viewer #5798

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

Merged
merged 17 commits into from
May 2, 2025
Merged

import diff viewer #5798

merged 17 commits into from
May 2, 2025

Conversation

nightwing
Copy link
Member

@akoreman
Copy link
Contributor

Thanks for the creating the PR, I think it looks really cool 🚀 I do have some UX questions:

  • Currently there is no way for users to select and copy code from the "+" diff blocks, do you think we should support that?
  • When using the cursor to select a selection within the editor, the "+" diff blocks get styled like they are selected, even though their content is not within the current selection (which I think is the right behavior), do you think that could be visually confusing for users?
    Screenshot 2025-04-22 at 14 31 31

I also think there is a bug with folding code which includes "+" diff blocks where content gets mashed-up (see line 216 and 217 in the image).

Screenshot 2025-04-22 at 14 34 50

@nlujjawal
Copy link
Contributor

Awesome PR diff view looks really nice. I was playing around and found some bugs

  1. alignment of fold icons is not consistent for a normal line vs a removed line.
Screenshot 2025-04-22 at 15 54 17 2. There is empty space in the bottom when the code is folded in the removed lines Screenshot 2025-04-22 at 15 54 33 3. There is a very little green color bar in the removed lines fold Screenshot 2025-04-22 at 15 55 24

@marinsokol5
Copy link
Contributor

Does it make sense to disable folding all together when viewing the diff?
Cause when added lines are inside of the current content, like in the one below, folding it would hide the added line. It's kind of unclear what would be the best UX for it, so might indeed be best to not allow it at all.
Screenshot 2025-04-23 at 10 43 48

We can also check how Monaco inline diff acts when folding, but I would lean on the side of disabling.

@xyos
Copy link
Contributor

xyos commented Apr 23, 2025

Great work 🥳 I found another bug with the collapse unchanged lines option and it's related to unfolding with a similar behavior to the one @akoreman reported:

image

@marinsokol5
Copy link
Contributor

Is dual numbering in the gutter intentional?

@nightwing nightwing force-pushed the simple-diff-viewer branch from bce6ded to 936ed98 Compare April 23, 2025 15:06
@nightwing nightwing force-pushed the simple-diff-viewer branch from 70ea6e5 to 7ec9e82 Compare April 28, 2025 10:43
@nightwing
Copy link
Member Author

I have fixed most of the issues with folds except the case when fold starts outside of changed range and ends inside one, perhaps we should indeed disable folding in that case.
I do not want to disable folding altogether, since it is useful for collapsing unchanged lines

For the accept/reject feauture, i think the easiest solution will be to modify align method to include widgets for these too.

I've also added ability to select text inside the green zones, but not sure if we should limit selection to one green zone, or to allow to select as if we are in a diffview with new code editable. In principle using the approach we could make both old and new editable in inline view.

@akoreman
Copy link
Contributor

I think being able to select text inside the green zones is a nice addition, I did spot some quirks with it:

  • If you double click the code in the green block it will apply a selection to the editor above the block
Screen.Recording.2025-04-28.at.15.39.09.mov
  • if I drag my mouse from a green block onto the editor, while mousedown it shows like it is selecting all of the code but then we you release the mouse nothing is actually selected
Screen.Recording.2025-04-28.at.15.44.21.mov
  • I don't seem to be able to actually copy the selected text to my clipboard (tried in FF on Mac)

Copy link

codecov bot commented May 2, 2025

Codecov Report

Attention: Patch coverage is 57.94393% with 405 lines in your changes missing coverage. Please review.

Project coverage is 86.84%. Comparing base (e08d4fe) to head (8f77471).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
src/ext/diff/base_diff_view.js 42.66% 254 Missing ⚠️
src/ext/diff/diff_view.js 48.50% 69 Missing ⚠️
src/ext/diff/inline_diff_view.js 72.85% 60 Missing ⚠️
src/layer/decorators.js 70.00% 9 Missing ⚠️
src/ext/diff/gutter_decorator.js 74.19% 8 Missing ⚠️
src/ext/diff/diff_test.js 94.73% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5798      +/-   ##
==========================================
- Coverage   87.34%   86.84%   -0.51%     
==========================================
  Files         605      611       +6     
  Lines       44620    45558     +938     
  Branches     7301     7493     +192     
==========================================
+ Hits        38975    39563     +588     
- Misses       5645     5995     +350     
Flag Coverage Δ
unittests 86.84% <57.94%> (-0.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@marinsokol5 marinsokol5 merged commit 93433f0 into master May 2, 2025
1 of 3 checks passed
@marinsokol5 marinsokol5 deleted the simple-diff-viewer branch May 2, 2025 14:40
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.

7 participants