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

[LineChart] Added ability to specify x and y keys as functions #11

Merged
merged 1 commit into from Oct 19, 2021

Conversation

michaldudak
Copy link
Collaborator

This gives the possibility to use a function to select data for x/y axes. The previous way (field selection by string) is still possible.

const xDomain = getExtent(data, (d) => d[xKey], xDomainProp);
const yDomain = getExtent(data, (d) => d[yKey], yDomainProp);
let xGetter: (record: Record) => X;
if (typeof xValueSelector === 'string') {
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be simpler to just support a function, or is it worth keeping support for strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It doesn't add much complexity in code and perhaps someone may find it useful, so I'd keep it.

@mbrookes mbrookes merged commit 2ec03e8 into mbrookes:charts Oct 19, 2021
@michaldudak michaldudak deleted the charts/xyAccessors branch October 20, 2021 08:05
@mbrookes
Copy link
Owner

mbrookes commented Oct 20, 2021

Sorry @michaldudak I had to revert - it breaks the markers on the line charts due to the change of context variable name. There's also a demo using the old prop name. I should have actually tested it before committing. 😅

It would also be good to make this change for all the components for consistency, and add a docs demo of its use.

mbrookes added a commit that referenced this pull request Oct 20, 2021
@mbrookes
Copy link
Owner

mbrookes commented Oct 20, 2021

Also d3 calls these types of functions "accessors" should we follow suit in the prop name, e.g. xAccessor, yAccessor? (I notice you did for the branch name...)

@michaldudak
Copy link
Collaborator Author

Good point. I'll change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants