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.

Sign up for GitHub

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

[docs] Fix multiple console.error messages on charts docs #14554

Merged
merged 3 commits into from
Sep 10, 2024
Merged

[docs] Fix multiple console.error messages on charts docs #14554

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
232a82c
Fix multiple console.error messages on docs
JCQuintas Sep 9, 2024
01979f7
Merge commit 'dd98382e7b1657235c12c1334e44a68fa003bd4a' into fix-cons…
JCQuintas Sep 9, 2024
9abd5b4
revert basic scatter changes
JCQuintas Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion docs/data/charts/lines/LineDataset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const customize = {
height: 300,
legend: { hidden: true },
margin: { top: 5 },
stackingOrder: 'descending',
};

export default function LineDataset() {
Expand Down
1 change: 0 additions & 1 deletion docs/data/charts/lines/LineDataset.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ const customize = {
height: 300,
legend: { hidden: true },
margin: { top: 5 },
stackingOrder: 'descending',
};

export default function LineDataset() {
Expand Down
2 changes: 1 addition & 1 deletion docs/data/charts/scatter/BasicScatter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export default function BasicScatter() {
},
{
label: 'Series B',
data: data.map((v) => ({ x: v.x1, y: v.y2, id: v.id })),
data: data.map((v) => ({ x: v.x1, y: v.y2, id: `${v.id}b` })),
Copy link
Member

Choose a reason for hiding this comment

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

I've two questions

  • Is something preventing us to use number as an item id? I assume user with an autoincrement id will have a similar issue
  • Is their a specific reason for adding a b at the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is something preventing us to use number as an item id?

Probably not, we would just have to ensure the numbers don't overlap, like 1000 + v.id

Is their a specific reason for adding a b at the end?

I just went with the simples way of making this work,
Series B => {id}b

Copy link
Member

@alexfauquette alexfauquette Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do not manage to reproduce the error. I reverted the change on localhost, and I see not message in the console while playing with this demo.

I assume this one can be reverted

Copy link
Member Author

Choose a reason for hiding this comment

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

🤔 true, I suppose I got confused with the logs while doing the other scatter issue 🙃

},
]}
/>
Expand Down
2 changes: 1 addition & 1 deletion docs/data/charts/scatter/BasicScatter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export default function BasicScatter() {
},
{
label: 'Series B',
data: data.map((v) => ({ x: v.x1, y: v.y2, id: v.id })),
data: data.map((v) => ({ x: v.x1, y: v.y2, id: `${v.id}b` })),
},
]}
/>
Expand Down
2 changes: 1 addition & 1 deletion docs/data/charts/scatter/BasicScatter.tsx.preview
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
},
{
label: 'Series B',
data: data.map((v) => ({ x: v.x1, y: v.y2, id: v.id })),
data: data.map((v) => ({ x: v.x1, y: v.y2, id: `${v.id}b` })),
},
]}
/>
2 changes: 1 addition & 1 deletion docs/data/charts/scatter/ColorScale.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,6 @@ function getGaussianSeriesData(mean, stdev = [0.5, 0.5], N = 50) {
Math.cos(2.0 * Math.PI * chance.floating({ min: 0, max: 0.99 })) *
stdev[1] +
mean[1];
return { x, y, z: x + y, id: i };
Copy link
Member

Choose a reason for hiding this comment

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

Why adding the mean.join(',') ?

If using indexes causes key to be inconsistent, we could add the seriesId in the key than striking the demo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why adding the mean.join(',') ?

Was a quickfix for having unique ids, the index is already there, adding the mean, which is unique for every call of this function, should make the ids/keys unique overall as well.

Keep in mind these are not in multiple series, they are in a single series, hence why the key=id=i was being an issue

If using indexes causes key to be inconsistent, we could add the seriesId in the key than striking the demo.

Tbh I have no idea why we need the id, I suppose it is to prevent duplicate keys? It feels unnecessary though, like we could use an internal id instead of requiring it, but if the user provides it, we should use what they provide.

Copy link
Member

@alexfauquette alexfauquette Sep 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, I did not see that the function was called multiple time to geenrate the data array. I thaugh it it was called once per series

data: [
      ...getGaussianSeriesData([-1, -1]),
      ...getGaussianSeriesData([-1, 1]),
      ...getGaussianSeriesData([1, 1]),
      ...getGaussianSeriesData([1, -1]),
    ]

Tbh I have no idea why we need the id,

For sure we could have a default id generation. The only issue is when user start to modify their data. In such case if they do not provide ids by themself, we have no way to properly define key attribute

return { x, y, z: x + y, id: `${mean.join(',')}${i}` };
});
}
2 changes: 1 addition & 1 deletion docs/data/charts/scatter/ColorScale.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,6 @@ function getGaussianSeriesData(
Math.cos(2.0 * Math.PI * chance.floating({ min: 0, max: 0.99 })) *
stdev[1] +
mean[1];
return { x, y, z: x + y, id: i };
return { x, y, z: x + y, id: `${mean.join(',')}${i}` };
});
}
1 change: 1 addition & 0 deletions docs/data/charts/styling/CustomOverlay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ function LoadingOverlay() {

return (
<LoadingReact
key={index}
x={xScale(item)}
width={bandWidth}
y={bottom - barHeight}
Expand Down
1 change: 1 addition & 0 deletions docs/data/charts/styling/CustomOverlay.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ function LoadingOverlay() {

return (
<LoadingReact
key={index}
x={xScale(item)}
width={bandWidth}
y={bottom - barHeight}
Expand Down