Implement #3722 Add date type to work with error bars

[mshvarts]

Issue: #3722

Not gonna say I didn't use GPT, which I did, but it worked for me as you can see:

I already did this in my repo and built it locally.
I just took the time to make this pull request out of care for others people having this problem and to contribute to this wonderful plugin.

PS: I tried running tests to make sure it doesn't break anything but they were already failing on my machine even before i made any changes. (So merge at your own risk)

[mshvarts]

Just saw few tests are failed, I haven't had time to look closely, since I haven't figured out how to run a single test instead of the whole suite, but I think it means few tests need to be changed and not due to an issue in the code.

[gvwilson]

Thanks for the PR @mshvarts - can you please update the tests as well? Once that's done I'll see if I can find a reviewer. Thanks - @gvwilson

[mshvarts]

Thanks for the PR @mshvarts - can you please update the tests as well? Once that's done I'll see if I can find a reviewer. Thanks - @gvwilson

Sorry I work full time and don't have time now. I can try to look into it later if you let me know how to run individual tests on my machine? Every time I tried adding flags it just ran the whole test suite instead and takes forever to complete.

[emilykl]

Thanks for the PR @mshvarts - can you please update the tests as well? Once that's done I'll see if I can find a reviewer. Thanks - @gvwilson

Sorry I work full time and don't have time now. I can try to look into it later if you let me know how to run individual tests on my machine? Every time I tried adding flags it just ran the whole test suite instead and takes forever to complete.

Hi @mshvarts, thank you for the PR!

You can run a single test filename by passing the filename, e.g. npm run test-jasmine bar_test.js. That's probably the best way to try to reproduce these failures on your local machine, since as you mentioned the full test suite takes a long time to run.

Within a file, you can also run just a single test by changing it( to fit(, or describe( to fdescribe( (Jasmine docs ref).

I'll try to find some time in the next few days to look at the test failures; we have a few tests which are sometimes flaky so the failures may or may not be due to these changes.

[oskarr]

I looked into the test failure briefly (as I need this functionality), and removing all changes except the one on line 29 makes it pass the hover_label test, but still renders error bars for me (I have not run all the other tests).

For 'date' axes, the ax.c2l and ax.l2c conversions call Lib.ensureNumber, but when the axis type is 'log' the conversions instead convert to and from log, which seemingly causes the hover_label test failure. Perhaps calling Lib.ensureNumber directly is better (if it is needed at all)?

[emilykl]

@mshvarts Yes, @oskarr is correct, I did some testing of my own and the change on line 29 is the only change technically needed to enable these error bars. The other changes adding the ax.c2l and ax.l2c conversions actively cause incorrect results for log axes (because it causes the error values to be calculated on log values rather than on raw values). And they have no effect on date axes anyway.

So reverting all the changes except for the line 29 change is a good start.

However the resulting outcome only works when the error bar size is specified in milliseconds, and it's possible we should support string values as well, so more work will be needed.

[alexcjohnson]

it's possible we should support string values as well

I'd suggest not blocking on that. As long as we want to support milliseconds as a format - which I think is the right call, and the only plausible interpretation of numbers here - then we can add string handling as a separate effort. Or more generally, other ways to provide this data; strings are good if we can settle on a clear and flexible standard for specifying time intervals, but another option would be to split it into two attributes: units (that default to milliseconds but could be any other time unit) and a number.