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

Display issue closed reason in timeline events #1223

Merged
merged 2 commits into from
Oct 7, 2022

Conversation

Fs00
Copy link
Contributor

@Fs00 Fs00 commented Aug 16, 2022

This PR keeps OctoDroid up with the changes GitHub has recently made to the "closed" issue status:

  • the "closed as completed" and "closed as not planned" information has been added to the closed timeline event
  • the IssueActivity/Fragment color has been changed to purple if the issue is closed as completed.
    For closed as not planned issues I've kept the classic red color - even if GitHub UI uses dark grey - for consistency with the closed red icon that is already used in the app, and because the grey color used by GH is very similar to the grey used for PR draft status (and draft issues may be a thing in the future, according to the issue linked above)
  • the Closed tab in the Issues list has had its color changed from red to purple (since issues are by default closed as completed)
    • following the same logic, the color of the My issues list has been changed to purple when showing closed issues
  • small extra: I've added the reason why the issue has been locked in the locked timeline event when available

I haven't added the ability to close an issue as not planned since it seems it can't be done via the REST API: we need to use the closeIssue GraphQL mutation. Also, when we introduce that feature, the UI will need to be revised since it's possible to change the closed reason of an issue by "re-closing" it.

A few more technical details that will be useful while reviewing this:

  • in timeline events, the stateReason field is null if the issue was closed before the introduction of closed reasons. The GitHub UI displays the event with closed as completed when stateReason is completed or null
  • the Issue stateReason field is never null for closed issues: it's set to completed for both closed as completed issues and issues that were closed before the introduction of closed reasons

private static final int[][] ISSUE_HEADER_COLOR_ATTRS = new int[][] {
{ R.attr.colorIssueOpen, R.attr.colorIssueOpenDark },
{ R.attr.colorIssueClosedCompleted, R.attr.colorIssueClosedCompletedDark }
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the issue activity distinguishes between 'open', 'closed' and 'completed' now, I think the issue list activity should do the same.

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 don't agree on this... "completed" is the reason why an issue has been closed, it isn't a third state.
Furthermore, if we follow this logic, the three tabs should be called "Open", "Not planned" and "Completed". But even in that way it would look overkill IMHO, considering how the GitHub UI displays the new close reasons.

Copy link
Collaborator

@maniac103 maniac103 Sep 1, 2022

Choose a reason for hiding this comment

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

'Completed' basically is the equivalent of the 'merged' PR state: 'closed as done' ... treating it that way in the UI doesn't seem like overkill to me, but rather more consistent?

So IMO it should be either 'show all closed issues, with red header' or 'distinguish between closed and completed, using the respective colors'.

Copy link
Contributor Author

@Fs00 Fs00 Sep 1, 2022

Choose a reason for hiding this comment

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

'Completed' basically is the equivalent of the 'merged' PR state: 'closed as done'

Mmhh... I believe it's a bit of a stretch to see "closed as completed" in the same way as the Merged state.
The GitHub UI emphasizes it differently (there's no "Completed" purple pill for issues like the Merged pill for PRs) and there's also the fact that all old issues are displayed as "closed as completed" while they might have been closed for other reasons... this is why I don't like having a "hard" separation in the app.
My idea in this PR was to replicate the GH web UI behavior in the app, with the only difference that not planned is red instead of grey.

show all closed issues, with red header

I guess it could be fine, but what about using purple for all issues instead? Red is not used anymore for issues in GitHub UI and the completed-purple has become the "default" closed color.

@Fs00
Copy link
Contributor Author

Fs00 commented Oct 7, 2022

@maniac103 I've removed the commits that change the header colors, can this be merged as-is for now?

@Fs00 Fs00 changed the title Add initial support for issue closed reasons Display issue closed reason in timeline events Oct 7, 2022
@Fs00 Fs00 requested a review from maniac103 October 7, 2022 12:12
@maniac103 maniac103 merged commit fec3807 into slapperwan:master Oct 7, 2022
@Fs00 Fs00 deleted the issue-colors branch October 7, 2022 16:37
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 this pull request may close these issues.

2 participants