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.

Sign up for GitHub

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

Jump to bottom

Filter for 'Merged' #182

Merged
merged 9 commits into from
Jul 24, 2023
Merged

Filter for 'Merged' #182

merged 9 commits into from
Jul 24, 2023

Conversation

joyngjr
Copy link
Contributor

@joyngjr joyngjr commented Jul 20, 2023

Summary:

Fixes #167

Currently, PRs cannot be filtered by its 'merged' status even though merged PRs are displayed when no filters are set.

However, as issues should only be filtered with status 'all/open/closed', 'all' is used as a default status when user tries to achieve an invalid combination (status: merged + type: all/PR -> status: merged + type: issue).

Option to select 'merged' status is removed when the current selected type is 'issue'.

Type of change:

  • ✨ New Feature/ Enhancement
  • 🐛 Bug Fix

Changes Made:

  • Adds an option to filter by 'merged' status for types 'all' and 'pull request'

Screenshots:

filter.status.screen.recording.mov

Proposed Commit Message:

Allow users to filter their PRs by 'Merged' status.

Previously, merged PRs are only shown when status is selected as 'All'.

Let's allow users to filter their PRs by 'Merged' status.

Checklist:
  • I have tested my changes thoroughly.
  • I have created tests for any new code files created in this PR or provided a link to a issue/PR that addresses this.
  • I have added or modified code comments to improve code readability where necessary.
  • I have updated the project's documentation as necessary.

@joyngjr joyngjr changed the title More filter options Filter for 'Merged' Jul 20, 2023
@joyngjr joyngjr marked this pull request as draft July 20, 2023 08:40
@joyngjr joyngjr marked this pull request as ready for review July 20, 2023 08:54
gycgabriel
gycgabriel requested changes Jul 23, 2023
View reviewed changes
Copy link
Collaborator

@gycgabriel gycgabriel left a comment

Choose a reason for hiding this comment

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

Small changes

src/app/shared/filter-bar/filter-bar.component.ts Outdated
Comment on lines 83 to 91
/**
* Changes status to a valid, default value when an incompatible combination of type and status is encountered.
*/
checkValidCombination() {
if (this.dropdownFilter.status === 'merged' && this.dropdownFilter.type === 'issue') {
this.dropdownFilter.status = 'all';
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add some logic to change the dropdown filter type to Pull Request if Merged is selected?

I don't think we need this logic? Because it seems impossible to select Merged when Issue is selected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it so that PR will be selected if merged is selected!

I think we will still need to check for the combination 'issue + merged' because users can select 'pr + merged' and then change the type from pr to issue.

Another possibility could be to remove the 'issue' option when 'merged' is selected, but this would mean more coupling and I'm not sure if this would cause the behaviour to deviate too much from the original.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better to not hide the Issue option, since that filter is more important than filtering by status. Thanks this implementation is good

gycgabriel
gycgabriel approved these changes Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@gycgabriel gycgabriel left a comment

Choose a reason for hiding this comment

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

LGTM

@gycgabriel gycgabriel merged commit d92b63a into CATcher-org:main Jul 24, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gycgabriel gycgabriel gycgabriel approved these changes

Assignees
No one assigned
Labels
s.toReview
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No way to filter by merged PRs
2 participants
@joyngjr @gycgabriel