Skip to content

/ react-vis

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

Loosen `tickValues` propTypes to support `Date`s in time series #1096

Open
wants to merge 1 commit into
base: master
from
Open

Loosen `tickValues` propTypes to support `Date`s in time series #1096

wants to merge 1 commit into from

Conversation

@agwells
Copy link

agwells commented Jan 25, 2019

A time series can have JS Date objects (or epoch numbers) as
values. (See issue #1083)
@agwells
Loosen `tickValues` propTypes to support `Date`s in time series
Loading status checks…
03297f7 
A time series can have JS `Date` objects (or epoch numbers) as
values.
@CLAassistant
Copy link

CLAassistant commented Jan 25, 2019 •
edited

CLA assistant check
All committers have signed the CLA.
@mcnuttandrew
mcnuttandrew requested changes Jan 25, 2019
View changes
Copy link
Collaborator

mcnuttandrew left a comment

This is great! Would you mind documenting this by modifying one of the showcase examples and writing a test?
@agwells
Copy link
Author

agwells commented Jan 25, 2019

I took a look into writing a test for it, as I was writing up the merge request. But I could find a suitable existing test to use as a template. There are some tests in tests/components/axes-tests.js that use the tickValues prop, but they're only testing it with empty or single-value arrays.
@mcnuttandrew
Copy link
Collaborator

mcnuttandrew commented Jan 25, 2019

Hmmm, well we tend to be somewhat integration-y in our tests. You might consider making a new showcase component (maybe forking misc/time-chart or one of the axes/X.js examples) and then you could write a text check in the manner of those tests in the line-series-chart.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcnuttandrew mcnuttandrew

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@agwells @CLAassistant @mcnuttandrew
You can’t perform that action at this time.