Skip to content
Open
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,21 @@ export default function SaveSpreadsheetButton({

const getCsvProps = useCallback(
(csvCase: SpreadsheetSaveOptionId.COPY_CSV | SpreadsheetSaveOptionId.EXPORT_CSV) => {
const gridCsvFunction =
csvCase === SpreadsheetSaveOptionId.COPY_CSV
? gridRef.current?.api.getDataAsCsv
: gridRef.current?.api.exportDataAsCsv;
const isCopy = csvCase === SpreadsheetSaveOptionId.COPY_CSV;
const gridCsvFunction = (params?: any) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i guess the type here (params?: ValueGetterParams)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be CsvExportParams.

const exportParams = {
...params,
shouldRowBeSkipped: (rowParams: any) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(owParams: ShouldRowBeSkippedParams)

const rowData: { rowType?: string } = rowParams.node?.data;
// remove lines used to calculate
return rowData?.rowType?.includes('calculation');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
return rowData?.rowType?.includes('calculation');
return rowData?.rowType?.includes(CalculationRowType.CALCULATION);

and "includes" works but I think it would be cleaner and more long time effective to create a specific utility function like :

export function isCalculationRowType(rowType: string | undefined): rowType is CalculationRowType {
    return rowType === CalculationRowType.CALCULATION || rowType === CalculationRowType.CALCULATION_BUTTON;
}

},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
};

return isCopy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you don't need to define cont varaiable isCopy, you can used directly like
csvCase === SpreadsheetSaveOptionId.COPY_CSV ?
gridRef.current?.api.getDataAsCsv(exportParams)
: gridRef.current?.api.exportDataAsCsv(exportParams);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

True but I think this is clearer to avoid big ternaries.

? gridRef.current?.api.getDataAsCsv(exportParams)
: gridRef.current?.api.exportDataAsCsv(exportParams);
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (!gridCsvFunction) {
console.error('Csv API is not available.');
return;
Comment on lines 96 to 98

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this will never happen again because now in your code gridCsvFunction is never undefined (even if it may return undefined). The test should be updated to really test the api availability.

Expand Down
Loading