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

docs: 🗒️ improve data structure sections #29

Merged
merged 11 commits into from
Nov 6, 2024

Conversation

lloydrichards
Copy link
Contributor

Goals/Scope

There is a bit of documentation that wasn't updated with v3.0.0 that needs to be updated. This PR will update the documentation to expand on the data structure sections of several of the charts to be more clear on what the data structure should look like. This will be a good opportunity to also update the documentation to be more clear on how to structure the data for the charts.

At the moment it looks like there are unique data structures for the following charts:

  • Stacked Bar Chart
  • Grouped Bar Chart
  • Nested Bar Chart
  • Stacked Area Chart
  • Sankey Diagram
  • Stacked Pyramid

With the rest of the charts use a similar flat data structure so will leave those as is.

Description

Most of the documentation is now in place, but would be good to have a check. Have added lots of code examples to match the sample charts so its clearer on how to build the data structure for the chart.

I've also added a series of unit tests to validate the data structure for the charts. This will help to ensure that the data structure is correct for the charts, as well as were a way to better explore the data outside of the chart implementation.

I also found a bug with the nested stacked bar chart that resulted in the chart needing to be rendered twice. Have fixed this bug, but will need to bump the version to v3.0.1 to include the fix.

Comments

Note

Found another bug when writing the unit tests for the Stacked Bar Chart that shows that stackedBarVerticalData doesn't sort the data by index. I haven't fixed this bug yet, as it will possibly change how these charts are implemented but will not it down in the project board and have added an inline comment in the code.


  • Timeframe: [1 week]

@lloydrichards lloydrichards self-assigned this Oct 29, 2024
Comment on lines +11 to +15
[
[0, 10, data: {...}],
[0, 20, data: {...}]
key: "key1"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

At first I was like… hey, this is not valid JavaScript. But then I noticed that it's difficult to express the data as a JS expression (while keeping it readable) :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its even weirder when you want to express it in typescript:

interface StackedData<T extends Record> extends Array<number> {
  0: number;
  1: number;
  data: T;
  key: string;
}
Screenshot 2024-10-30 at 22 08 05

Comment on lines +38 to +47
expect(stackedData[0]).toHaveProperty("key");
expect(stackedData[0]).toHaveProperty("index");
expect(stackedData[0]).toHaveProperty("0");

expect(stackedData[0][0][0]).toBeDefined();
expect(stackedData[0][0][1]).toBeDefined();

expect(stackedData[0][0][1]).toBe(stackedData[1][0][0]);

expect(stackedData[0][0]).toHaveProperty("data");
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it wouldn't better communicate what transformation actually happens when you assert on the expected structure, eg. (and I'm making stuff up here…):

const tidyData = [
  { xValue: 1901, yValue: 8702, category: "Ausländer" },
];

const stackedData = [
  [0, 8702]
];

expect(stackLayout(tidyData)).toBe(stackedData)

Hm, I guess the same issue as with https://github.com/StatistikStadtZuerich/sszvis/pull/29/files#r1823357416, it's difficult to express the output as a JS expression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking this at the beginning but was trying to rationalise the test as not being a validation of the transformation, but a confirmation of the structure that the other components need to map the data 🤷 Since some of the functions are expecting the .data prop on the stack the test is really to see if that property exists on the array 😓

@lloydrichards lloydrichards merged commit 7ee3528 into master Nov 6, 2024
3 checks passed
@lloydrichards lloydrichards deleted the docs/data-structures branch November 6, 2024 08:50
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.

2 participants