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

[#1473] Checkbox not working properly in break down by file type #1475

Conversation

gerhean
Copy link
Contributor

@gerhean gerhean commented Mar 12, 2021

Fixes #1473

this.getFiltered() is not called when checkAllFileTypes
is modified.

This is regression introduced by PR [#1440]

Let's call this.getFiltered() when checkAllFileTypes
is modified.

@gerhean gerhean requested a review from a team March 12, 2021 09:16
@Tejas2805
Copy link
Contributor

@gerhean there still seems to be some bug as noticed in deployment.

Untick on any filetype that is not displayed in a specific repository. The line count doesn't change but the length of the contribution bar still changes for the specific repository.

  1. Java ticked.

ticked

  1. Java unticked.

unitcked

Copy link
Contributor

@Tejas2805 Tejas2805 left a comment

Choose a reason for hiding this comment

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

The initial bug is fixed but can notice other bugs.

@gerhean
Copy link
Contributor Author

gerhean commented Mar 13, 2021

| Untick on any filetype that is not displayed in a specific repository. The line count doesn't change but the length of the contribution bar still changes for the specific repository.

This other bug is caused by the computed property avgContributionSize in summary.js not being a constant when report is filtered (it is a variable which depends on the line count of every single repository displayed).

I don't think this bug will be as simple to fix at least, since avgContributionSize is a computed property with side effects, and there seems to be other functions which use this avgContributionSize.

I feel the best way to fix this is to make each summary chart its own component, such that we can compute a single repoAvgContributionSize per summary chart.

Another way would be to fix avgContributionSize when the report loads into the browser and not change it afterwards.

Copy link
Member

@dcshzj dcshzj left a comment

Choose a reason for hiding this comment

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

The commit message should be getFiltered instead of just filtered, otherwise LGTM.

Copy link
Contributor

@0blivious 0blivious left a comment

Choose a reason for hiding this comment

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

LGTM, in the PR message, please also specify this is regression introduced by PR [#1440]

@0blivious
Copy link
Contributor

@Tejas2805 I think I went through this before when I implemented this feature and at that time we agreed that this problem is tolerable.

@gerhean
Copy link
Contributor Author

gerhean commented Mar 25, 2021

@Tejas2805 So can the other bugs be fixed in a separate PR if needed?

@gerhean gerhean requested a review from Tejas2805 March 25, 2021 05:47
Copy link
Contributor

@Tejas2805 Tejas2805 left a comment

Choose a reason for hiding this comment

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

LGTM!

@fzdy1914
Copy link
Member

`this.getFiltered()` is not called when `checkAllFileTypes` is 
modified. This is regression introduced by PR #1440.

Let's call `this.getFiltered()` when `checkAllFileTypes` is 
modified.

@fzdy1914 fzdy1914 merged commit 21b4f9c into reposense:master Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkbox not working properly in break down by file type
5 participants