Enable Export with Excel Format #214
Conversation
🦋 Changeset detectedLatest commit: 60a9584 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Can you write tests for this file
There was a problem hiding this comment.
What type of test would you suggest? the data is binary, so we really cannot build one like is done for CSV's, I thought about it but it felt like there was little value add for doing this based on this constraint.
There was a problem hiding this comment.
You can read the stream via https://www.npmjs.com/package/read-excel-file, which is nice as you don't need to write a file in the test, keeping it pure.
There was a problem hiding this comment.
This seems ok, I am not in love with it because we are using another library to test this one in essence but seems like a start.
There was a problem hiding this comment.
It's by the same author so I think that reduces the risk somewhat
There was a problem hiding this comment.
I agree, it just feels like we are testing the library itself instead of anything novel about how it is used, I will add it though as it is at least something. Thanks for the feedback on this.
There was a problem hiding this comment.
My thinking is that there is some code that can benefit from testing which is not library related. Another approach is to take a writeData callback that looks like
const readStream = await writeXlsxFile(excelData, { columns })
for await (const chunk of readStream) {
stream.write(chunk)
}
That way you don't have to read the xlsx file but can still test this function.
There was a problem hiding this comment.
Reminder that this still needs to be done.
There was a problem hiding this comment.
This is in the current PR and the tests is using it as such.
positiveVibes4all
force-pushed
the
feature/ADD-EXCEL-EXPORT-ABILITY
branch
from
d65129f to
ce54bf9
Compare
packages/export/src/excel.js
Outdated
| } | ||
| let columns = _.map( | ||
| () => ({ | ||
| width: 50, |
There was a problem hiding this comment.
What is the default behavior for width? If the default behavior is not smart, we should consider setting the width to the largest value of the following: the length of the column name or 50.
There was a problem hiding this comment.
default is not smart, 50 seemed like a reasonable size to start with and wrapping the data set to true.
There was a problem hiding this comment.
I wrote some code to keep track of the longest value and then use the column name as the shortest
There was a problem hiding this comment.
Reminder that this still needs to be done.
- Added smart column width - Added Sticky Header - Added Background Color like CSV for Header
|
Code looks good. Pending approval as I cannot test this with the application due to some bug in the spark PR. |
|
Coverage after merging feature/ADD-EXCEL-EXPORT-ABILITY into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Summary
This PR adds the capability to use an export with '.xlsx' format.