Conversation
TobyCyan
left a comment
TobyCyan
left a comment
There was a problem hiding this comment.
Thank you for making your code clean and readable with comments! Really liked how you encapsulated each anomaly type into their own classes. You could consider my comments that I left for this PR regarding naming conventions and typo issues. Otherwise, overall LGTM!
| isDraft: false | ||
| }); | ||
|
|
||
| export const OPEN_ISSUE_WITH_CLOSED_MMILESTONE = new GithubIssue({ |
There was a problem hiding this comment.
I could be wrong but is this a typo? If so maybe could fix this?
| export abstract class MilestoneAnomaly { | ||
| anomaly: string; | ||
|
|
||
| constructor(anomaly: string) { | ||
| this.anomaly = anomaly; | ||
| } | ||
|
|
||
| abstract getDescription(): string; | ||
| } |
There was a problem hiding this comment.
Thanks for creating an abstract base class to make this feature more extensible 😄 ! However since this class only holds an anomaly string, maybe could call this Anomaly instead? And have another MilestoneAnomaly that inherits from this. Considering we could have other kinds of anomalies in the future.
| } | ||
| } | ||
|
|
||
| export class GeneralMilestoneAnomaly extends MilestoneAnomaly { |
There was a problem hiding this comment.
Since the only difference between this class and the SingleMilestoneAnomaly class is that this holds multiple milestones instead of one, perhaps can call this MultipleMilestoneAnomaly instead? Do correct me if I'm understanding this wrongly!
|
Thank you for the suggestions and spotting the typo! I have updated the PR to fix the typo issue and created a new abstract class I did not rename the |
NorbertLoh
left a comment
NorbertLoh
left a comment
There was a problem hiding this comment.
I quite like this feature. It would be useful in ensuring the correct usage of milestones.
Perhaps instead of showing it in all the layouts, we can keep it to Group by -> Milestones.
Maybe the design of the box could be better.
Here are some inspirations you can take from
(I quite like this)
…to feature/milestone-anomalies-warnings
|
@NorbertLoh Hi, thank you for the feedback! I have made the changes accordingly. |
4 participants
Summary:
Fixes #459
Type of change:
Changes Made:
Implemented a new model
MilestoneAnomalyto represent a milestone anomaly.When milestone has one of the following anomalies, a warning will be shown above the issues and PRs section. The warning will display both the milestone title and its anomaly.
Milestone anomalies will only be displayed when users set the
Group Byfilter toMilestone. When set toAssignee, the warning card will not be displayed.Screenshots:
Before:
No warning is shown for milestone anomalies:
After:
When any milestones has anomalies:
When there are too many milestone anomalies, it will be kept at a fixed height. The component will become scrollable
Width of this component will follow the width of the issues/PRs section
When
Group Byfilter is set toAssignee, no milestone anomaly warnings will be shownProposed Commit Message:
Checklist: