mozilla-frontend-infra / firefox-health-dashboard

https://firefox-health-dashboard.netlify.com
Mozilla Public License 2.0
26 stars 68 forks source link

most charts not locking to timeDomain #466

Closed klahnakoski closed 4 years ago

klahnakoski commented 5 years ago

if there are no datapoints at either extreme of the timeRange, the chart does not show the full time range, only the range of time with points

grantlouisherman commented 4 years ago

hey @klahnakoski I can take at look at this issue. Could you provide a screen shot of what you are talking about?

klahnakoski commented 4 years ago

You can see the problem here:

https://health.graphics/android/tp6m?test=cold-loadtime&browserPlatform=fenix-g5&past=month&ending=2019-11-19

The chart should be showing a full month ending November 19, which means the first date should be October19. The chart starts on October26.

image

KaairaGupta commented 4 years ago

hey @klahnakoski ! is this issue still up? I would like to work on it

KaairaGupta commented 4 years ago

@klahnakoski I have set up my environment. Since this is my first contribution can you guide me as to how to proceed further?

grantlouisherman commented 4 years ago

Hey @KaairaGupta you can take it on. I figured out the bug is coming from how the program creates dates. I believe there is a date component you can look at.

KaairaGupta commented 4 years ago

@grantlouisherman can you please guide me as to which files i should look at?

klahnakoski commented 4 years ago

The problem can be seen here: In the moreProps.options.scales.xAxis, which is sent to the ChartJs library. The code responsible for setting this value is busted.

image

Looking for xAxes will reveal the code: And we now have a good hint about why the xAxes is not set properly.

image

Please notice: I am using the Firefox debugger, and I have the React debug tools installed.

vshkr99 commented 4 years ago

@klahnakoski if the issue has not been resolved yet, then can I take it up ? I have been looking upon this issue for a while now and would be happy to resolve it. @KaairaGupta if you are done with solving the issue, please let me know.

KaairaGupta commented 4 years ago

@vaideshshank if you have made any progress with it, you can work on it

klahnakoski commented 4 years ago

@vaideshshank This issue is still not resolved. Please go ahead and work on it.

vshkr99 commented 4 years ago

Sure I'll resolve it asap.

vshkr99 commented 4 years ago

For solving the problem related to time domain, there is a need to pass data.labels as prop in Chart component which is currently empty, due to which the scatter points are self adjusted along the time axis based upon existence of value at a particular timestamp.

Capture

Here I have passed the data.labels object as prop in Chart component which restricts the values of the scatter points in the given time range. For month as the time range parameter, I have passed 30 date objects as labels. By doing this, the data points get distributed according to their timestamps and y values. Similarly done for other time range parameter. I just want to know if this approach is fine or if there are any other suggestions by the collaborators for improving the solution, please do let me know before I raise a PR.

If everything seems fine, I'll raise a PR within a day or two.

klahnakoski commented 4 years ago

@vaideshshank I think your solution is too complicated: The ChartJS is perfectly capable of generating the axis we want, as long as we send it the min and max. Here is an example using y-axis:

https://www.chartjs.org/docs/latest/axes/cartesian/linear.html#step-size

You can see the code attempting to set this min and max, but there is a logic error:

https://github.com/mozilla-frontend-infra/firefox-health-dashboard/blob/master/src/vendor/components/chartJs/utils.js#L110

vshkr99 commented 4 years ago

Yes, I think the xDomain value is getting null in this https://github.com/mozilla-frontend-infra/firefox-health-dashboard/blob/master/src/vendor/components/chartJs/utils.js#L110 due to which the default value is returned having no min or max . Thanks for the suggestion, I will update the changes in PR.