-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add iso code columns to the GHG Emissions module's data downloads #1668
Conversation
PR Summary
|
@@ -52,6 +52,8 @@ export const getTableData = createSelector( | |||
return { [String(d.x)]: formatValue(d[c.value]) }; // year: value | |||
}; | |||
return yColumnOptions.map(c => ({ | |||
// Conditionally add ISO to the data when countries or regions is selected | |||
...(model === 'regions' && { iso: c.iso }), |
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.
Note: the model is also regions
when the user selects to show data by Countries
const activeColumnNames = activeColumns.map(c => c.value); | ||
const activeColumnNames = activeColumns | ||
.map(c => c.value) | ||
.filter(c => !hiddenColumns.includes(c)); |
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.
I'm not 100% sure about this.
I mostly wanted to avoid the activeColumns
implementation altogether to prevent bugs, as it seems to rely on the column labels to end with the NotShow
string. Because it affects other modules and due to time constraints I worked around this, though I believe it'd be ideal to refactor it in the future.
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.
Yes, maybe we can filter that column before it arrives to the table-component in the future
@@ -182,7 +185,7 @@ function TableComponent(props) { | |||
> | |||
<AutoSizer disableHeight> | |||
{({ width }) => | |||
(splittedColumns ? ( | |||
splittedColumns ? ( |
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.
No actual changes here, automated VSCode changes due to ESlint config.
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.
Thanks! Not sure if the hiddenColumns is the clearest option for the developers to understand this columns go only to the download, but as you say maybe this is the option with the less changes so probably the best for this case
This PR is the frontend counterpart to adding an ISO code column to data downloads.
While the Data Explorer's CSV downloads come from our API (with the exception of Pathways, which is downloaded from a third party API), the GHG Emissions CSV is generated by the frontend, based on the data passed onto the table.
Implementation choices
Because the data displayed and the data downloaded won't necessarily be the same anymore, I see two options:
getDownloadData
selectorgetTableData
could make use of it and simply remove theiso
in order to parse the data for the tableI feel this would be confusing for future development as it would deviate from the implementation in other modules
iso
to the existinggetTableData
selectortable
component to allow us to hide the new columnIn this PR I opted for the second option because:
getData
returns chart data,getTableData
returns table data, CSV downloads are still based on the latterCaveats
I'm not exactly the biggest fan of conditionally adding the iso to the data based on the model in this way (done because we'll only need the iso when the user has chosen to show the data by either countries or regions, but not sectors or gas) nor specifying the columns in this way .
I've pondered a similar implementation to the one in the data explorer, but it feels like it would be bug prone due to the usage of
GHG_TABLE_HEADER
in places such as here. Worth considering in the future, but perhaps not now due to limitations.How to test