-
Notifications
You must be signed in to change notification settings - Fork 20
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
DATAP-1533 stacked area chart class to functional #537
DATAP-1533 stacked area chart class to functional #537
Conversation
@@ -335,17 +335,17 @@ export const pruneIncompleteStackedAreaInterval = ( | |||
) => { | |||
const { from: dateFrom, to: dateTo } = dateRange; | |||
|
|||
const dataClone = cloneDeep(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what kind of issues were you seeing here, and I think you may not need this
This is doing the exact same thing at L342.
cloneDeep in our utils does the same thing too:
https://github.com/cfpb/ccdb5-ui/blob/main/src/utils/index.js#L209
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cloneDeep(data)
value was basically being passed in as a parameter from the component. So instead of doing that, I thought it made more sense to just pass in data
, then do the clone here. It cuts down on some of the data processing being done directly in the component, and eliminates a variable that's only used for this.
The same thing was also done for pruneIncompleteLineInterval()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work without adding this? This looks like it is doing the same thing twice
I would change L342 to
let filteredData = cloneDeep(data);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My apologoies for the late response (I looked at this yesterday, but forgot to comment here). Changing this results in a state mutation error. And when I tried to use the structuredClone
as stated in the comments, it doesn't appear to be available in Lodash. So it looks like we'll need to keep this as-is (at least for now).
Conversion of StackedAreaChart from class to functional component
Additions
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist