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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature remove-diff-signs #1866

Merged
merged 10 commits into from Mar 27, 2019
Merged

Add feature remove-diff-signs #1866

merged 10 commits into from Mar 27, 2019

Conversation

fregante
Copy link
Member

@fregante fregante commented Mar 19, 2019

Closes #1464

It can be disabled 馃巿

Test

Any commits on https://github.com/sindresorhus/refined-github/commits/master
Any reviewed block on #1783
Full diff on https://github.com/sindresorhus/refined-github/pull/1783/files

  inline diffs
- minus
+ plus
diff --git a/foo.c b/foo.c
index 30cfd169..8de130c2 100644
--- a/foo.c
+++ b/foo.c
@@ -1,5 +1,5 @@
 #include <string.h>

 int check (char *string) {
-    return !strcmp(string, "ok");
+    return (string != NULL) && !strcmp(string, "ok");
 }

Question

Where else do commits appear

Screenshot

screenshot

It's a single class added on body, using ajax+pageDetect is an overkill.

Fixes: going from Commits page to Commit
@fregante
Copy link
Member Author

fregante commented Mar 22, 2019

This is ready to merge, except I need suggestions on how to handle diffs and patches. This is how it looks currently.

Screenshot 2019-03-22 at 16 04 25

Notes:

  • ```diff and ```patch are indistinguishable from the dom
  • patch will show those ---/+++ at the start and they're not removable
  • in this case, -/+ are hidden but copy-pastable

Perhaps we should leave inline diffs/patches untouched.

@sindresorhus
Copy link
Member

Perhaps we should leave inline diffs/patches untouched.

Yeah, probably best. I don't think people would realize they could still copy-paste it and use it as a .diff with Git if they don't see the + and -.

@sindresorhus
Copy link
Member

The downside of hiding the diff symbols is that it now looks like all code is indented. Should we also move it to the left?

@sindresorhus
Copy link
Member

Where else do commits appear

Change suggestions: sindresorhus/package-json#51 (comment) (Works)

@fregante
Copy link
Member Author

The downside of hiding the diff symbols is that it now looks like all code is indented. Should we also move it to the left?

I'm hesitant to do that. Not all code blocks are indented the same way (some use padding-left, some plain text nodes) and other extensions might expect the space to be there:

Remember what we had to go through with the previous remove-diff-signs

@sindresorhus
Copy link
Member

Remember what we had to go through with the previous remove-diff-signs

Yeah... But that was slightly different from what I can remember. Then, we actually replaced some nodes. This would only be CSS changes.

Copy link
Member

@sindresorhus sindresorhus left a comment

Choose a reason for hiding this comment

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

Let's try it like this now and see how we feel about it after a while.

@fregante fregante merged commit 65a4ae2 into master Mar 27, 2019
@fregante fregante deleted the remove-diff-signs branch March 27, 2019 17:17
@fregante fregante mentioned this pull request Mar 30, 2019
@nesl247
Copy link
Contributor

nesl247 commented Apr 5, 2019

Commenting here since I was about to open a bug about it. The new spacing is much worse IMO than having the symbols. The spacing causes me to believe everything is formatted badly when reading the diffs and I have to remember that the space is there because of this feature.

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

Successfully merging this pull request may close these issues.

Add opt-in setting to remove diff signs
3 participants