Feature: configurable custom file exports #533
Conversation
There was a problem hiding this comment.
Some code style changes required but we should focus first on discussing shape of configuration. We have topic of localisation in different issue right now. Would be great if you could define which "localeOptions" are needed in your case - discussion about native vs numbro is still open.
| } | ||
|
|
||
| public equals(other: ClassShareOptions): boolean { | ||
| return other instanceof ClassShareOptions && this.valueOf() === other.valueOf(); |
There was a problem hiding this comment.
It's reference comparison, not equality check.
| }); | ||
| this.options = [...newOptionsCopy]; | ||
| } else { | ||
| this.options = [...shareOptionsDefaults]; |
There was a problem hiding this comment.
Why spread + array construction? Is it basically a this.options = shareOptionsDefaults equivalent?
| locale?: Locales; | ||
| } | ||
|
|
||
| export type ShareOptions = ShareOption[]; |
There was a problem hiding this comment.
Using immutable List would simplify some code later.
|
|
||
| if (options) { | ||
| const newOptionsCopy = options.map( option => { | ||
| option = { ...shareOptionsDefaults[1], ...option }; |
There was a problem hiding this comment.
Why [1]? Whole merging code here is convoluted here.
| finalLineBreak: "suppress", | ||
| columnOrdering: "keys-first", | ||
| lineBreak: "\r\n", | ||
| locale: |
There was a problem hiding this comment.
We shouldn't confuse locale and formatting options. We need to design this API in clean way.
| @@ -34,15 +37,13 @@ function findSeriesAndDerivation(name: string, concreteSeriesList: List<Concrete | |||
| return null; | |||
| } | |||
|
|
|||
| export default function tabularOptions(essence: Essence): TabulatorOptions { | |||
| export default function tabularOptions(essence: Essence, locales?: Locales): TabulatorOptions { | |||
There was a problem hiding this comment.
You use locales like it is required.
| const type = `${getMIMEType(fileFormat)};charset=utf-8`; | ||
| const blob = new Blob([datasetToFileString(dataset, fileFormat, options)], { type }); | ||
| if (!fileName) fileName = `${new Date()}-data`; | ||
| fileName += `.${fileFormat}`; | ||
| filesaver.saveAs(blob, fileName, true); // true == disable auto BOM | ||
| } | ||
|
|
||
| export function datasetToFileString(dataset: Dataset, fileFormat: FileFormat, options?: TabulatorOptions): string { | ||
| export function datasetToFileString(dataset: Dataset, fileFormat?: FileFormat, options?: TabulatorOptions): string { | ||
| const optionsCopy = { ...options }; |
There was a problem hiding this comment.
Possible spread on undefined.
| @@ -37,19 +37,20 @@ export function getMIMEType(fileType: string) { | |||
| } | |||
| } | |||
|
|
|||
| export function download({ dataset, options }: DataSetWithTabOptions, fileFormat: FileFormat, fileName?: string): void { | |||
| export function download({ dataset, options }: DataSetWithTabOptions, fileFormat: FileFormat, fileName?: string, custom?: boolean): void { | |||
| const { openOn, onClose } = props; | ||
|
|
||
| const { openOn, onClose, customization } = props; | ||
| const shareOptions = customization.shareOptions; |
| const dataSetWithTabOptions = getDownloadableDataset(); | ||
|
|
||
| let dataSetWithTabOptions = getDownloadableDataset(); | ||
| dataSetWithTabOptions.options = { |
There was a problem hiding this comment.
Extract function that deals with construction of this object.
|
Let's extract bug fix to another PR, so we could merge it earlier. Rest of it should wait till we discuss what is needed and how we should achieve it. For reference #504 |
|
Bugfix extracted to #536 |
KarolakJakub
changed the title
There was a problem hiding this comment.
The proposed direction for different export formats adds unnecessary complexity and will be confusing for end user. End user should export exactly what is presented on the screen, with preserved number and date formats. The Turnilo end users experiences will be consistent as it is now (I hope).
|
|
||
| - title: "excel friendly export to TSV" # name displayed in the share menu | ||
| format: # "tsv" | "csv" | ||
| separator: # "\t" | "," | ... |
There was a problem hiding this comment.
What is the reason to define format AND separator? Format defines separator implicitly.
| separator: # "\t" | "," | ... | ||
| lineBreak: "\r\n" | ||
| finalLineBreak: # "include" | "suppress" | ||
| columnOrdering: # "keys-first" | "as-seen"; |
There was a problem hiding this comment.
Export should always preserve what is presented on the screen. If not user will be confused, please remember that end user is not aware of configuration.
| lineBreak: "\r\n" | ||
| finalLineBreak: # "include" | "suppress" | ||
| columnOrdering: # "keys-first" | "as-seen"; | ||
| locale: |
There was a problem hiding this comment.
As Adrian mentioned, there is open issue #504. And again, export should always preserve what is presented on the screen. It applies also to number and date formats. If some formats are not currently available on UI we should focus to remove existing limitations and add more formatting options.
Turnilo should be easy to understand for end user. How to
There was a problem hiding this comment.
I've replied in the #504 . 👍
Thank you for review, I will apply and push requested changes soon.
Regarding exports reflecting what's presented on screen/app - please note that this is already not the case. Turnilo is modifying date format on export
Consider this export made on latest master - data is in a different format and value is rounded up in the app view.
Comma used as decimal separator is our main business need - Turnilo uses dot to separate fractional part of a number which causes issues when opened in Excel in our locale (pl-PL).
There was a problem hiding this comment.
Yep, date & number formats are fixed in CSV/TSV. Currently there is no way to configure format for date dimensions in Turnilo, so dates on UI are formatted in the most concise way. We also put some effort (#288) to keep date formatting consistent across whole app, the export is the only exception with ISO formatting.
On the other hand for metrics, administrator is able to configure default format and user is able to change the configured format on UI, but the formatting is totally ignored by export CSV/TSV :(
Right now I don't know how to design date/number formatting in consistent way. I feel convinced that formatting on UI and formatting in the exports could be different but the formatting is rather related to the given dimension/measure not to particular export.
Perhaps we should enhance existing formatting configuration to datetime and numeric dimensions in similar way to measures. But for sure start with good design before implementation step.
This PR includes a feature that allows to create custom export options, as described in configuration.md - users will be able to add multiple configurable export types with different formatting, locales etc. If no defined exports are found within the config, Turnilo will use the default 'Export to TSV' and 'Export to CSV' ones.