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

[charts] Allow dataset to be used with ScatterChart #14915

Merged
merged 8 commits into from
Oct 15, 2024

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Oct 10, 2024

Resolves #14333

Uses a single object signature. As it more accurately leverage typescript to force users to provide all keys when the object exists.

type DatasetKeys = {
  x: string,
  y: string,
  z?: string,
  id: string
}

type ScatterChart = {
  datasetKeys?: DatasetKeys
}

@JCQuintas JCQuintas added new feature New feature or request component: charts This is the name of the generic UI component, not the React module! labels Oct 10, 2024
@JCQuintas JCQuintas self-assigned this Oct 10, 2024
@mui-bot
Copy link

mui-bot commented Oct 10, 2024

Deploy preview: https://deploy-preview-14915--material-ui-x.netlify.app/

Updated pages:

Generated by 🚫 dangerJS against 1e4ae95

Copy link

codspeed-hq bot commented Oct 10, 2024

CodSpeed Performance Report

Merging #14915 will not alter performance

Comparing JCQuintas:scatter-dataset (1e4ae95) with master (fe69246)

Summary

✅ 3 untouched benchmarks

@alexfauquette
Copy link
Member

@JCQuintas I like your idea of dataKeys = { x: 'xKey', y: 'ykey', id: 'idKey'}

Another more flexible option could be to introduce a getValue: (item) => ({ x, y, id }). But the first one has the advantage to be consistent with other charts and other libraries

@JCQuintas
Copy link
Member Author

@JCQuintas I like your idea of dataKeys = { x: 'xKey', y: 'ykey', id: 'idKey'}

Another more flexible option could be to introduce a getValue: (item) => ({ x, y, id }). But the first one has the advantage to be consistent with other charts and other libraries

Yeah, a function is more flexible. We should probably migrate into functions eventually though, but I didn't feel confident to do so now 😅

@alexfauquette
Copy link
Member

I didn't feel confident to do so now

But are you ok to update the PR with dataKeys object instead of multiple properties?

@JCQuintas
Copy link
Member Author

I didn't feel confident to do so now

But are you ok to update the PR with dataKeys object instead of multiple properties?

yep

docs/data/charts/scatter/scatter.md Outdated Show resolved Hide resolved
packages/x-charts/src/ScatterChart/formatter.ts Outdated Show resolved Hide resolved
Comment on lines 24 to 31
const data = !hasStringKeys
? (seriesData.data ?? [])
: (dataset?.map((d) => {
const x = d[xDataKey!]!;
const y = d[yDataKey!]!;
const z = d[zDataKey ?? ''];
const id = d[idDataKey!]!;

Copy link
Member

Choose a reason for hiding this comment

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

Since datasetKeys should contain x, y, and id if defined, some ! could be removed

Suggested change
const data = !hasStringKeys
? (seriesData.data ?? [])
: (dataset?.map((d) => {
const x = d[xDataKey!]!;
const y = d[yDataKey!]!;
const z = d[zDataKey ?? ''];
const id = d[idDataKey!]!;
const data = !seriesData?.datasetKeys
? (seriesData.data ?? [])
: (dataset?.map((d) => {
const x = d[seriesData?.datasetKeys.x];
const y = d[seriesData?.datasetKeys.y];
const z = seriesData?.datasetKeys && d[seriesData?.datasetKeys.z];
const id = d[seriesData?.datasetKeys.id];

Copy link
Member Author

Choose a reason for hiding this comment

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

Its kind of required still

            const x = d[seriesData?.datasetKeys!.x];
            const y = d[seriesData?.datasetKeys!.y];
            const z = seriesData?.datasetKeys?.z && d[seriesData?.datasetKeys!.z!];
            const id = d[seriesData?.datasetKeys!.id];

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made it closer to my initial solution, but made it cleaner, so we don't need all those ! 😅

Copy link
Member

Choose a reason for hiding this comment

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

I added a commit to extract the datasetKeys to avoid all those seriesData?.datasetKeys. And instead of getting a boolean, I get the list of missing properties to have a more precise error message.

I also updated the docs demo to avoid overflow between y-axis ticks and the label

JCQuintas and others added 4 commits October 14, 2024 15:22
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants