mozilla / telemetry-dashboard

Web-frontend for aggregated telemetry data
telemetry.mozilla.org
Other
89 stars 113 forks source link

Configurable evoBucketIndex implementation #571

Closed ab0092 closed 6 years ago

ab0092 commented 6 years ago

@chutten i have done the requested changes.

ab0092 commented 6 years ago

@chutten the minor issues can be addressed now itself for any major changes we can work on those on a separate item. First for the index out of bound issue we can put a validation check for it, secondly for the bucketindex value for boolean bucket lists what is it that you suggest we go ahead with??

chutten commented 6 years ago

Yes, I think validating the index (perhaps when you're checking it for > 0on line 355) is a good one to include here.

I'm also interested to see what you have in mind for fixing the boolean case.

ab0092 commented 6 years ago

@chutten for the boolean case if only the text of yLabel is to be changed then we can assign to '% TRUE' based on the BucketIndex. For the index validation i think having the check on line 362 is a better place to validate the index with the hist.values list length.

chutten commented 6 years ago

Good point!

ab0092 commented 6 years ago

@chutten so i will add the index validation and for the boolean instance is my idea good to go with?

chutten commented 6 years ago

Yes, please do. Or you could make it so boolean snaps the bucket index to 0, or you can make it so that if it is boolean and non-0 it uses the yLabel = 'bucket - ' + bucketIndex; style. There are options to go with, I encourage you to test your preferred one out to make sure it works out well.

ab0092 commented 6 years ago

@chutten i will go with your suggestion and only set yLabel text to '% False' if the kind is boolean and BucketIndex is 0 else it will show yLabel = 'bucket - ' + bucketIndex;

ab0092 commented 6 years ago

@chutten i am made the necessary changes please have a look if anything more is to added.

ab0092 commented 6 years ago

@chutten i think it will be better if you raise the issue instead of me because i am not sure if i will be able to describe the issue as it needs to be done for the other users to understand. I might read the issue and try to understand and may be perhaps add an issue myself later on by taking some reference and assistance from you.

chutten commented 6 years ago

Alrighty, I've filed #572. Please take a look and see if it needs any clarification (or if you'd like to work on it!)

ab0092 commented 6 years ago

@chutten thanks for creating #572. I have gone through it and will work start working on it and provide an update once i have done the necessary changes. Thank you for the assist on resolving this issue.

ab0092 commented 6 years ago

@chutten just one last assist i require that is when i am creating a PR from my forked repo its still shows my previous approved & merged commits as part of the new Pull Request. Is there any issue from my end while generating the PR ? Thanks..

chutten commented 6 years ago

I think maybe it is because you haven't rebased your fork against the latest from mozilla/gh-pages. Try adding mozilla's repo as a remote and rebasing your branch against its gh-pages branch?