Make file view in Pull Request show "file added/modified/deleted" status #3438

Not a bad idea. We could just cross deleted files and make added files somehow. The rest of them would be unchanged.

The unfortunate part is that I don't think the page tells us whether a file is added at all, so we'd have to query the API for them ☹️

Can't we see from here?

Indeed we can! But that's also loaded on demand, here's the fragment for example: https://github.com/sindresorhus/refined-github/pull/3420/show_toc?base_sha=0706bf161fa63580d628887d780af61901187703&sha1=0706bf161fa63580d628887d780af61901187703&sha2=1c5494edcf4be3678cf47f9c7ca2b4b98f0b7403

Indeed we can! But that's also loaded on demand, here's the fragment for example: https://github.com/sindresorhus/refined-github/pull/3420/show_toc?base_sha=0706bf161fa63580d628887d780af61901187703&sha1=0706bf161fa63580d628887d780af61901187703&sha2=1c5494edcf4be3678cf47f9c7ca2b4b98f0b7403

We can make the same network call that is triggered by clicking the button. I looked in the DevTools network tab and saw a call to /show-toc?...other_get_parameters.

Not a bad idea. We could just cross deleted files and make added files somehow. The rest of them would be unchanged.

I like @fregante's idea. I think adding an icon like in #3439 is redundant because the diffstat blocks are distinct enough. Also this can apply to all commits, not just PR.

because the diffstat blocks are distinct enough

👎

Well, how about change the filename's color like in git diff

I am ok with that.

@fregante WDYT?

What it may look like:

The code:

document.querySelectorAll('.diffstat[aria-label]').forEach(diff => {
  const filename = diff.nextElementSibling
  const label = diff.getAttribute('aria-label')
  const noAddition = label.includes(': 0 additions')
  const noDeletion = label.includes('& 0 deletions') || label.includes('Empty file added')

  if (!noAddition && noDeletion) {
    filename.classList.remove('link-gray-dark')
    filename.classList.add('text-green')
  } else if (noAddition && !noDeletion) {
    filename.classList.remove('link-gray-dark')
    filename.classList.add('text-red')
  }
})

Just because a file does not have deletions does not mean that its a new file...

Just because a file does not have deletions does not mean that its a new file...

So what's wrong with the code? I got what you mean now

I think there's a lot less inference required if you parse the results of the "jump to..." network call. For example, the modification icon, which looks like this: Has the following SVG:

<svg class="octicon octicon-diff-modified select-menu-item-icon" title="modified" viewBox="0 0 16 16" version="1.1" width="16" height="16" aria-hidden="true">...</svg>

We could simply parse the title="modified" attribute.

if you parse the results of the "jump to..." network call

But parsing diff blocks works on regular commits as well.

If it is accepted to add to regular commits we can take it from.

If it is accepted to add to regular commits we can take it from.

That means separate logic for the two pages (the PR one has lazyload logic while the one on commit page doesn't), and also it will need to loop through A list of elements to get status and then change B list of filenames, which is inefficient than just selecting diffblocks and matching substrings.

hat means separate logic for the two pages (the PR one has lazyload logic while the one on commit page doesn't), and also it will need to loop through A list of elements to get status and then change B list of filenames, which is inefficient than just selecting diffblocks and matching substrings.

???

Did you see the pr?

which is inefficient than just selecting diffblocks and matching substrings.

Again you cannot see if a a file was created or deleted from a diff block. So i am not sure what info you want to take from there.

I think the icons as designed in https://github.com/sindresorhus/refined-github/pull/3439 are clean and clear enough. Coloring the text might be a bit much.

@kidonng the information we’re targeting is not available on the the PR Files page. We’re only highlighting CREATED and DELETED files, not just “all green” and “all red” files.

As for the regular commits page, I think it’s as easy as

if isPRFiles
  await deferred
else
  // nothing, it’s already on the page

We’re only highlighting CREATED and DELETED files, not just “all green” and “all red” files

My fault. Sorry @yakov116!