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

Add support for issues closed as not planned #191

Merged
merged 13 commits into from
Nov 3, 2023
Merged

Add support for issues closed as not planned #191

merged 13 commits into from
Nov 3, 2023

Conversation

joyngjr
Copy link
Contributor

@joyngjr joyngjr commented Aug 5, 2023

Summary:

Fixes #186

Displays issues that are closed as not planned differently from issues which are closed as completed.

However, this differentiation is not applicable to the filter bar (i.e. filtering closed issues will display issues which are closed as completed and closed as not planned). I'm not sure if we should split 'closed' into 'closed as completed' and 'closed as not planned' in the filter bar as this distinction is only applicable to issues, and may not be compatible with the current design of the filter bar given the increased number of possible invalid combinations (eg. PRs which are closed as not planned, merged issues). Perhaps some refactoring for selection validation in the filter bar would be useful to account for the invalid combinations if more options are added?

Type of change:

  • ✨ New Feature/ Enhancement
  • 🧪 Tests Update

Changes Made:

  • Update graphql library version so schema includes IssueStateReason
  • Update queries to retrieve information on stateReason
  • Add placeholder issue-draft icon as the icon for 'issue closed as not planned' is not in the @primer/octicons library
  • Differentiate between issues that are closed as completed and closed as not planned

Screenshots:

image

Proposed Commit Message:

Differentiates between issues closed as completed and issues closed as not planned.

Previously, all closed issues are displayed in the same way.

Let's display closed issues differently based on its reason for being closed.

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 Closed as not planned Add support for issues closed as not planned Aug 6, 2023
gycgabriel
gycgabriel reviewed Aug 15, 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.

No worries about the filter at this point, this is just for visual purposes.

We can use the octicon 'skip'

Do open issues have default stateReason as reopened or is it null?

@joyngjr
Copy link
Contributor Author

joyngjr commented Aug 17, 2023

@gycgabriel Nope, they do not, so its stateReason is null!

Eclipse-Dominator
Eclipse-Dominator requested changes Aug 18, 2023
View reviewed changes
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

LGTM except to remove some auto-generated files from graphql

graphql/graphql-types.ts Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be ignored by .gitignore as it is auto-generated on ng:serve:web.

If you encounter typing issue with graphql-related stuff, u can rerun npm run ng:serve:web to update this file.

src/app/shared/issue-pr-card/issue-pr-card-header/issue-pr-card-header.component.ts Outdated
@@ -18,14 +18,18 @@ export class IssuePrCardHeaderComponent {
getOcticon() {
const type = this.issue.issueOrPr;
const state = this.issue.state;
const stateReason = this.issue.stateReason;

switch (true) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to convert this into a regular if-else statement at this point with the increasing number of checks.

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 made the changes! Though, would it be better to abstract out the checking logic into another service class instead because of the layers of nesting?

seetohjinwei and others added 5 commits August 19, 2023 10:33
@seetohjinwei @joyngjr
Immediately display page data that has loaded (CATcher-org#179)
9ae5660 
Previously, the page would show loading spinners, with no content,
until all the data has been loaded.

Let's
* Display the page data once it is loaded.
* Display loading spinners below the content, until all data has been
  loaded.
* Change `Sync` to a spinner while the content is being loaded.
@joyngjr
Add support for private repos (CATcher-org#190)
2464b62 
Let's allow users to use WATcher for their private repositories.
@chia-yh @joyngjr
Update labels without refresh (CATcher-org#165)
a2c2ba4 
Update labels without needing to refresh

Labels are only fetched on initialization, requiring users to refresh
in order for labels to update.

Let's change it so labels are updated periodically, and when the Sync
button is clicked.
@joyngjr
Convert switch statement to if-else, remove untracked files,
66c696a 
remove trailing whitespace
@joyngjr
try to remove untracked files again
65b7508
Eclipse-Dominator
Eclipse-Dominator requested changes Sep 18, 2023
View reviewed changes
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

LGTM. Let's fix the merge conflict.

src/app/core/services/factories/factory.auth.service.ts Outdated
@@ -30,12 +32,23 @@ export function AuthServiceFactory(
// githubService,
// userService,
// issueService,
// labelService,
// phaseService,
// githubEventService,
// titleService,
// logger
// );
// }
console.log(logger);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
console.log(logger);

not part of this PR but let's remove the debugging statement.

@joyngjr
Copy link
Contributor Author

joyngjr commented Oct 27, 2023

@Eclipse-Dominator @gycgabriel Hi, I've made the changes as described. Could I get your help to review this PR?

Eclipse-Dominator
Eclipse-Dominator approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@Eclipse-Dominator Eclipse-Dominator left a comment

Choose a reason for hiding this comment

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

LGTM!

@Eclipse-Dominator Eclipse-Dominator merged commit 36b2ebe into CATcher-org:main Nov 3, 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 left review comments

@Eclipse-Dominator Eclipse-Dominator Eclipse-Dominator 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.

Add support for Issue Closed as Not Planned
5 participants
@joyngjr @Eclipse-Dominator @gycgabriel @seetohjinwei @chia-yh