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

[dev-6596] data driven grouped tables #985

Merged
merged 5 commits into from
Jan 4, 2024
Merged

Conversation

joshlacey
Copy link
Collaborator

@joshlacey joshlacey commented Dec 28, 2023

added pie chart example and added grouped table example to storybook

@joshlacey joshlacey changed the title added pie chart example and added grouped table example to storybook [dev-6596] data driven grouped tables Dec 28, 2023
Copy link
Collaborator

@adamdoe adamdoe left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't catch that this was just a draft. I think its looking good so far. I didn't dive into the table stuff deep, I'll trust that that's looking good since you're in the weeds with it. The two things that did come up for me were 1) deletions from EditorPanel. I'm wondering if they showed as unused but are actually needed. It seemed like a good chunk was stripped out and I'm pretty sure the DataSuppression component and remove series functions needs to stay in. So maybe double check deletions there. 2) The DataImport file was changed to use .json. Does that break csv imports?

packages/chart/src/components/EditorPanel.jsx Outdated Show resolved Hide resolved
packages/chart/src/components/EditorPanel.jsx Outdated Show resolved Hide resolved
packages/chart/src/types/ChartConfig.ts Outdated Show resolved Hide resolved
@joshlacey
Copy link
Collaborator Author

@adamdoe

  1. there was a data suppression section in the EditorPanel code that was commented out and I think that was the only place these were used. I'm usually of the opinion that it's better to remove unused code, it's alway present in the git history so if you need it in the future you can grab it then. I'm fine with putting it back though if you feel otherwise, doesn't affect this PR at all, was just doing some cleanup.

  2. I'll need to double check about the csv import. I made the change because it was throwing an error because the url didn't end in .json, which is specific to Socrata. I'll need to make some change along these lines. I could remove this from this PR though as it doesn't affect the grouped tables. I was just trying to test external endpoints.

@joshlacey
Copy link
Collaborator Author

@adamdoe some of the work to pull out the helper functions from CdcChart aren't needed for this PR. I started doing it because I needed to figure out something else, I think it simplifies CdcChart a lot though so I left it in. Let me know if you want me to put that change in a different PR.

Also I'm waiting for an okay from my DBA about the data structure for the table, once I get that I'll move this out of draft.

@joshlacey joshlacey marked this pull request as ready for review January 4, 2024 17:01
@adamdoe adamdoe merged commit 2a2a1e7 into dev Jan 4, 2024
@joshlacey joshlacey deleted the dev-6596-grouped-tables branch August 15, 2024 20:27
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