diff icon indicating copy to clipboard operation
diff copied to clipboard

Feature: expand context

Open RudolfMan opened this issue 5 years ago • 1 comments

The ability to expand the context of the code around the change.

There are definitely stuff to improve.. especially in my frontend spaghetti JS (not sure if it makes sense to rewrite Diffs views to live_view since that page isn't meant to be live)

So currently onclick on any button "expanUp" or "expandDown" script tries to read numbers from lines before and after the button and makes the request: GET /diff/:package/:version/expand/:from_line/:to_line/:right_line?file_name=<file_name>

In the backend we unpack that file from, extract the requested chunk and render the html

To be able to unpack particular file I propose a patch to hex_core to add :hex_tarball.unpack/3 to accept the list of files. see https://github.com/hexpm/hex_core/pull/95

expand_context

RudolfMan avatar Jun 10 '20 10:06 RudolfMan

Thanks for the PR, it looks really great. Just a few points that I think we can improve:

Fetching a tarball and unpacking it every time a section expanded is a bit wasteful. It would be better to upload the individual files to the bucket after the tarball is unpacked for diffing and when we expand only fetch that file instead of the whole tarball.

The expand buttons should not be displayed when there is nothing to expand:

Screen Shot 2020-06-10 at 15 03 04 Screen Shot 2020-06-10 at 14 59 51

I am also a bit concerned about the amount of javascript, but I am not sure how well liveview would work on the potentially large diffs we show. If we start using liveview I think it would be worth showing smaller diffs and ask users to expand the diff to show more files.

For now the javascript is fine but if you want to try out liveview for this page in a separate PR then go ahead :).

ericmj avatar Jun 10 '20 13:06 ericmj