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

Add single-diff-column-selection feature #3698

Merged
merged 23 commits into from
Nov 11, 2020
Merged

Add single-diff-column-selection feature #3698

merged 23 commits into from
Nov 11, 2020

Conversation

cheap-glitch
Copy link
Member

@cheap-glitch cheap-glitch commented Nov 4, 2020

Thanks for contributing! 🍄

  1. LINKED ISSUES: Closes allow multiline selection in diff view without having both views intermingled #2765

  2. TEST URLS: https://github.com/sindresorhus/refined-github/pull/3695/files

  3. SCREENSHOT:

demo-02

Not sure if this is the best approach for this, but it works. Side note: should we also disable selection on "expandable lines" too? The ones in the middle of files can end up being selected, which is probably not what the user wants.

EDIT: There was some slight but noticeable lag with huge diffs, so I rewrote the feature to target one diff table at a time, since users will probably always copy from one file at a time.

@fregante
Copy link
Member

fregante commented Nov 4, 2020

Oh hell yeah. Put a 🔥 in the readme description.

@fregante
Copy link
Member

fregante commented Nov 4, 2020

A few notes:

I’d suggest handling the CSS with a single class. For example you can apply a rgh-select-left on the table element, and then a CSS rule applies the user-select. This is much easier to handle.

You can probably merge the two delegated events into one, a just detect which side was selected in the handler.

You can use element.closest('tbody') instead of that function with a loop 🎉

Make sure this only applies when the diff is split, not unified.

@fregante
Copy link
Member

fregante commented Nov 4, 2020

Or instead of the class, you can use a data-rgh-select=right attribute for example, so removing it means always delete table.dataset.rghSelect and adding it means setting a single value (instead of searching removing both left/right classes)

@fregante
Copy link
Member

fregante commented Nov 4, 2020

should we also disable selection on "expandable lines" too?

I don’t think that the alternative is particularly better, the copied text would just be missing part of code without any indication of it.

@cheap-glitch
Copy link
Member Author

I applied your suggestions @fregante, the code is much cleaner now.

source/features/better-diff-selection.tsx Outdated Show resolved Hide resolved
source/features/better-diff-selection.tsx Outdated Show resolved Hide resolved
source/features/better-diff-selection.tsx Outdated Show resolved Hide resolved
source/features/better-diff-selection.tsx Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
Co-authored-by: Federico <me@fregante.com>
@fregante
Copy link
Member

fregante commented Nov 4, 2020

I'm not sure about what's causing, but sometimes in Chrome the selection fails. Maybe it's because I'm selecting "between lines"? Unsure

a212

@yakov116 yakov116 changed the title Allow multiline selections on a single side of split diffs Add single-diff-column-selection feature Nov 5, 2020
@cheap-glitch
Copy link
Member Author

cheap-glitch commented Nov 7, 2020

I'm not sure about what's causing, but sometimes in Chrome the selection fails. Maybe it's because I'm selecting "between lines"? Unsure

I can't reproduce it neither in Firefox nor Chromium, so it might be a Chrome bug.

@fregante
Copy link
Member

fregante commented Nov 10, 2020

I think I got it. The current code covers all the cases and works in both Firefox and Chrome. Try it out and let me know if you see any issues.

@cheap-glitch
Copy link
Member Author

It's working great, nicely done! 💯

The only (tiny) snag I saw was that dragging the selection in Chromium ends up taking both sides of the diff with it. This doesn't happen in Firefox though, so it's probably a browser-specific bug.

@fregante
Copy link
Member

dragging the selection in Chromium ends up taking both sides of the diff with it

It shows both sides, but only our side is eventually pasted (at least on Mac)

I looked into ways to properly exclude the selection, but multi-cursor selection isn't supported in Chrome. Short of fully detaching those DOM nodes, I don't see another way.

Hey, if copy/paste works, that's already better than now!

Copy link
Member

@yakov116 yakov116 left a comment

Choose a reason for hiding this comment

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

🔥 thank you so much.

This is awesome!!

@sindresorhus I think this deserves a tweet. This was such a pain point of using GitHub diff in split view.

@fregante fregante merged commit ea6c3bf into refined-github:master Nov 11, 2020
@cheap-glitch cheap-glitch deleted the better-diff-selection branch November 11, 2020 22:27
@timotheecour
Copy link

@cheap-glitch I updated the extension but it doesn't seem to work, do you need to tag a new release first?

@yakov116
Copy link
Member

There will be a release tomorrow

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.

allow multiline selection in diff view without having both views intermingled
4 participants