CSV download include selected or all columns #552 #595
Conversation
Added the download CSV for selected and All PR's
npm install file-saver
|
|
There was a problem hiding this comment.
I think you can you can just include all the columns by default instead of actually respecting the current column selections.
Someone who's analyzing the whole thing can filter our/hide specific columns in their spreadsheet, but the current implementation requires them to manually change the column selection to include everything.
Additionally, please share screenshots/screen recordings for visual changes. :)
|
@vishalmaurya850 I see you've requested a review, but I don't see any changes since my last review. |
|
It was by mistake, give me some time |
Created CSV headers dynamically and also created a blob and trigger download and mapped the columnHeaders dynamically
Removed File Saver
|
Please review the changes |
| {/* Map through the columnHeaders object to dynamically generate table headers */} | ||
| {Object.entries(columnHeaders).map(([key, label]) => ( | ||
| <TableCell key={key} sx={{ minWidth: '40%', p: CELL_PAD, py: 1.5 }}> | ||
| <TableSortLabel | ||
| direction={conf.field === 'commits' ? conf.order : 'asc'} | ||
| active={conf.field === 'commits'} | ||
| onClick={() => updateSortConf('commits')} | ||
| direction={conf.field === key ? conf.order : 'asc'} | ||
| active={conf.field === key} | ||
| onClick={() => updateSortConf(key as keyof PR)} | ||
| > | ||
| Commits | ||
| {label} | ||
| </TableSortLabel> | ||
| </TableCell> | ||
| )} | ||
| ))} |
There was a problem hiding this comment.
While this is concise, this makes the order of the column headers susceptible to unexpected change.
The order of object also appears to not be guaranteed to the order of insertion:
https://stackoverflow.com/a/5525820
You may use a Map.
Regardless, it still doesn't account for the fact that not every header had the exact same styling properties. Some had a minWidth, some had no styling, some had nowrap, some had centered alignment.
This combined with a lack of screenshots, and a lack of tests, makes me think you didn't actually properly test this PR at all.
In think for your purposes, the simplest overall change would be to use the property names (like created_at) as headers instead of using formatted names (like Created At). That way, you retain the type safety without touching any of the existing code.
|
Hey @vishalmaurya850, still looking into it? |
|
No i am done with it |
|
I am unable to understand what the issue is because what you are telling is already in the code of middleware and I understood middleware is downloading the csv files perfectly |
Added the download CSV for selected and All PR's
Linked Issue(s)
Issue No. #552
Acceptance Criteria
When I hit 'Download CSV', I get a csv with either
a) the columns currently configured
or
b) all possible columns