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

Extra features for large PR workflow #175

Open
q-willboulter opened this issue Sep 18, 2021 · 4 comments · May be fixed by #176
Open

Extra features for large PR workflow #175

q-willboulter opened this issue Sep 18, 2021 · 4 comments · May be fixed by #176

Comments

@q-willboulter
Copy link

I just moved to Github from Gitlab and this extension is really great, thanks for building it!

I often have to review very large pull requests and there are a few features that help me quite a lot in my workflow, so I cloned the project and had a go at adding them. I was wondering if you're interested in having any of these:

  • Option to filter by the full file path instead of just file name. I use this to review things like api, src/main and src/test separately rather than go file by file alphabetically
  • Option to filter out file diffs as well as the tree view (so the diffs on the right hand side always match what's in the tree). This means I can filter to e.g. api and then scroll through all the api related changes
  • A button to click "viewed" on all currently visible files. I use this to register that I've reviewed all the api stuff after scrolling through it, rather than clicking on each individual file

I put these features as optional settings in the options page, with them disabled by default.

Happy to raise a PR but wanted to check if any of these sound useful first. If not that's totally fine, I can see why you'd want to avoid the behaviour of the extension spilling out into other areas of the screen such as the file diffs! Also, I'm going to be using this a lot for work so I'll be more than happy to help maintain it

Below is a gif to show the features together. The benefits aren't very obvious on small pull requests, but I struggled to find really large PRs in open source projects!

Animation

@berzniz
Copy link
Owner

berzniz commented Sep 18, 2021

Hi! Thanks for all the ideas and showing it in the gif. Welcome to GitHub as well.

I like all 3 ideas and would love to see a PR for each one separately.

  1. The filtering should just be the default. It makes sense to filter by the full path.
  2. I love this. Filtering out the files as well makes a lot of sense. I'm wondering how this works with back and forward buttons of the browser as the URL may point at a hidden file.
  3. I'm less sure of how this will work. This is a bit specific but let's see how it works out. Can be opt-in via the options.

Small PRs = will be easier to verify, test and merge

@q-willboulter
Copy link
Author

Okay great - I will get you over some PRs as soon as possible.

I will flip 1. so it's enabled by default, do you want me to add options for all of them still so they can be disabled/enabled?

For 2. my current implementation is really basic, I just set style.display = 'none' on all of the diffElement references that don't match the filter

  1. Might be less generally useful but when there's a subset of files that all have small/minor changes I like to skim them and then submit all so they are out of the way

I have a bunch of other ideas I'd like to try to but they get progressively more niche, these seemed like the most generally applicable 😀 The main other thing I would use if it was available:

Collapsible "advanced filters" section above the main search bar, which could contain:

  • number of lines changed range picker. I like to quickly eliminate all of files with minor changes sometimes and then get down to the ones with the big changes and review these in my IDE)
  • preset filters that can be customised in options html (e.g. user can set up text filters they use in their workflow via the options and then just click them instead of typing - mine might be api, service, test, main)
  • reset all filters button

@berzniz
Copy link
Owner

berzniz commented Sep 19, 2021

Great. I will review the PRs when they are available.

Regarding advanced filters - I can see an icon that opens up an filters popup/popout modal to accept the filters criteria. So this can also be its own feature

@wboult
Copy link

wboult commented Sep 19, 2021

Raised a few PRs, I will leave the button for checking all the boxes for now and see how I get on without it

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

Successfully merging a pull request may close this issue.

3 participants