no-unnecessary-split-diff-view - Restore on new PR & commit pages#8760
no-unnecessary-split-diff-view - Restore on new PR & commit pages#8760fregante merged 21 commits intorefined-github:mainfrom
no-unnecessary-split-diff-view - Restore on new PR & commit pages#8760Conversation
This comment was marked as resolved.
This comment was marked as resolved.
SunsetTechuila
force-pushed
the
split
branch
from
513f189 to
e07f18c
Compare
|
The old version also had a similar problem. IMHO it's still an improvement over a 50% of the screen being empty. |
|
everything seems to be fixed |
fregante
left a comment
fregante
left a comment
There was a problem hiding this comment.
It's very broken in every view for me. This PR looks like this:
In https://github.com/refined-github/sandbox/pull/50/files?diff=split the right side is just tighter, but the code is still there and badly wrapped (making the diff table taller)
https://github.com/user-attachments/assets/25e6ceb8-c311-4089-be83-213b6a27de43
In https://github.com/fregante/sandbox/pull/30/files the table the code is gone:
https://github.com/user-attachments/assets/c0acf755-e31e-458a-8fe6-23905f0ea52b
In refined-github/sandbox@c28cc8e?diff=split the review is tight and badly wrapped
https://github.com/user-attachments/assets/630845ab-d5ef-434c-9327-01fdf02608a7
could this be a safari thing? 🤔 
|
|
please try commenting width changes |
|
No I tried it in Chrome, I don't test PRs in Safari
? 
|
|
Here's my DOM, with the selector incorrectly targeting and hiding the 
|
|
You are not testing the latest changes. I've removed
commenting out * |
You only pushed those changes 14 minutes before I posted my review |
…ted changes td:nth-child(2/4) > .deletion/.addition doesn't select them yet, but it will at some point
| const table = tableBody.closest('table')!; | ||
| const columnsGroup = $('colgroup', table); | ||
| // Diff view is unified | ||
| if (columnsGroup.childElementCount !== 4) { |
There was a problem hiding this comment.
Is there no selector (other than has) that can detect this?
There was a problem hiding this comment.
I really tried to find one
fregante
left a comment
fregante
left a comment
There was a problem hiding this comment.
All the test URLs look good 🙌
| // Avoid selecting suggested deletions/additions | ||
| if (!$optional(':scope > tr > td:nth-child(2) > .deletion', tableBody)) { | ||
| table.classList.add('rgh-no-split-diff', 'rgh-only-additions'); | ||
| } else if (!$optional(':scope > tr > td:nth-child(4) > .addition', tableBody)) { |
There was a problem hiding this comment.
I should have used elementExists
There was a problem hiding this comment.
Would be great to write a no-restricted-syntax selector to detect $optional() or $() calls in if statements and include/exclude arrays. Claude is pretty good at this.
There was a problem hiding this comment.
Claude is pretty good at this.
Too bad I recently lost my free Copilot Pro access. Maybe I'll join some org on GitHub by 2026 to get it back
There was a problem hiding this comment.
I use Claude for free, this is 3 lines of outputted code
2 participants
fixes #8559
Test URLs
https://github.com/refined-github/sandbox/pull/50/files?diff=split
https://github.com/fregante/sandbox/pull/30/files
refined-github/sandbox@c28cc8e?diff=split
Screenshot