-
Notifications
You must be signed in to change notification settings - Fork 163
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
[#1899] Add graphic for commit diffstat #2010
Conversation
@nseah21 Thanks for taking this up. I personally prefer the other alternative (i.e., similar to the contribution bar), which is more consistent with RepoSense. Plus RepoSense is supposed to complement what GitHub does, rather than duplicate it. |
Hi @nseah21, your code changes look great for a first attempt at creating the GitHub style diffstat! The reason we prefer the cgit style is that we can reuse the same code we have for the contribution bars below each ramp chart for creating the diffstat as well. RepoSense/frontend/src/components/c-summary-charts.vue Lines 221 to 242 in 3878389
While you can copy over the same code for the diffstat, I would suggest that we move out this contribution bars code outside of We can discuss whether we want a toggle mode for the 2 styles in a separate PR, where we can make use of the current set of changes. Feel free to reach out to us if you have any questions with the |
@damithc @ckcherry23 Thank you for your comments. I have pushed some changes based on the initial feedback. I will look into using the code in |
@nseah21 Good progress. A few comments:
|
Great job so far and thanks for taking up this issue! Definitely agree with @ckcherry23 on re-using some of the code for the contribution bar - I believe it also has some logic that will help with what Prof mentioned above (overflow & scale). Also, as we are trying to unify the frontend to use TypeScript (#1936), it would be great if all new features/files could also be started in TypeScript. This can be done by adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @nseah21 with the progress! This wasn't an easy issue to take up and your progress on this has been impressive. The functionality works well, I left some comments w.r.t implementation details.
Regarding your questions, I think (1) can go either way - I don't think we necessarily need to follow the average across report like in summary, since that view contains charts across the whole report but the zoom view is restricted to a repo. Not too sure about (2), the non-linear scaling might be a bit misleading visually? Would love to get more opinions on these 2 questions!
@nseah21 remember to let reviewers know when the PR is ready for the next round of reviews. |
@vvidday I have attempted some changes for this feature based on the feedback. The PR is ready for the next round of reviews. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @nseah21 and apologies again for the late reply.
Will reply regarding the CSS overflow issue in the old thread above.
Also, before this pr is merged, it will be good to add cypress test cases for the new feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new changes look great @nseah21! I agree with David's comments and have a few more to add.
Regarding your questions,
For (1), according to prof's earlier comments, I think we should consider the average contribution size across the entire report.
- The same scale should be used for all commits in a report (not just one author) so that a visual comparison is possible. A 100 line commit of person A should be the same length as that of person B.
For (2), I also think the length of the diffstat should scale linearly as it's easier to comprehend for users.
Also, I think we should avoid the use of default red-colored bars on the summary chart contribution bars so that they are not confused with the deletions bar in the commit diffstat. I'd suggest using a shade of blue as the default for contribution bars.
I have just pushed the latest changes for the feature. Here is a summary of what has been changed:
May I have some guidance on what other tests should be added for the stacked-bar-chart component? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick iteration! I've left a few comments to address
@@ -1,7 +1,7 @@ | |||
describe('include merge commits in chart view', () => { | |||
it('show merge commits in summary chart', () => { | |||
// ramp chart should have merge commit slices | |||
cy.get('[title="[2023-03-03] Merge branch \'new-branch\' into cypress: +0 -0 lines "]') | |||
cy.get('[title="[2023-03-04] Merge branch \'new-branch\' into cypress: +0 -0 lines "]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look into this @nseah21. The date 2023-03-03
may not pass Cypress tests in your local machine but is the date that is required for the CI frontend tests to pass. The discrepancy is probably due to a time zone issue as the commit was made close to midnight. Perhaps we can create a new issue to investigate why the date varies on different machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting with the Cypress tests for this feature @nseah21. I think this file can be renamed to something like zoomView_diffstat.cy.js
.
Apart from checking if the diffstat has rendered, we can also check:
- if both green and red bars are rendered for a general commit with > 0 insertions and deletions.
- if the contribution bar is not visible for empty commits with 0 insertions/deletions.
- if the size of the green/red bars is proportional to the insertion/deletion size. I think a naïve way to do this is to check whether the green bar length is greater or less than the red bar length depending on the contribution size.
const contributionLimit = (this.avgContributionSize * 2); | ||
if (contributionLimit === 0) { | ||
return res; | ||
} | ||
const cnt = Math.floor(totalContribution / contributionLimit); | ||
for (let cntId = 0; cntId < cnt; cntId += 1) { | ||
res.push(100); | ||
res.push({ color: 'red', width: 100 }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…nto add-diffstat-graphic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nseah21, overall LGTM, just some minor things to tie up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, LGTM! Just remember to update your commit message with the latest changes and follow the guidelines.
I have updated the commit message accordingly. |
The following links are for previewing this pull request:
|
Fixes #1899
Add graphic for commit diffstat
Other information
NA