Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

Issue 546 - Visible columns and data misalignment in export #592

Merged
merged 4 commits into from
Sep 20, 2019
Merged

Issue 546 - Visible columns and data misalignment in export #592

merged 4 commits into from
Sep 20, 2019

Conversation

Marc-Andre-Rivet
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet commented Sep 19, 2019 •
edited
Loading

Fixes #546

  • documentation update for the new prop and default export / columns behavior

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-592 September 19, 2019 17:23 Inactive
Marc-Andre-Rivet
Marc-Andre-Rivet commented Sep 19, 2019
View reviewed changes
tests/cypress/tests/unit/exportUtils_tests.ts
{col1: 1, col2: 2, col3: 3}
{ col1: 1, col2: 2, col3: 'x', col4: 3 },
{ col1: 2, col2: 3, col3: 'x', col4: 4 },
{ col1: 1, col2: 2, col3: 'x', col4: 3 }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add one unused (hidden) column to the dataframe

Marc-Andre-Rivet
Marc-Andre-Rivet commented Sep 19, 2019
View reviewed changes
tests/cypress/tests/unit/exportUtils_tests.ts
@@ -266,7 +270,7 @@ describe('export', () => {
B2: {t: 'n', v: 2},
B3: {t: 'n', v: 3},
B4: {t: 'n', v: 2},
C1: {t: 's', v: 'col3'},
C1: {t: 's', v: 'col4'},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed col3 for col4 above

@alexcjohnson
Copy link
Collaborator

The poster in #546 thought the correct behavior should be to show all columns, hidden or not. This PR results in the export correctly matching what's displayed, which sounds like a reasonable default, but perhaps to make everyone happy we need to support either option somehow?

@Marc-Andre-Rivet
Copy link
Contributor Author

Marc-Andre-Rivet commented Sep 19, 2019 •
edited
Loading

I think I misunderstood the poster's meaning. I still think the correct default behavior is to ignore hidden columns.

(1) we already replicate the view’s content when it comes to sorting and filtering
(2) this is what we had in mind when implementing the feature, see #313
(3) w/o an alternative, including hidden columns would leave no alternative means of not including the columns if someone wanted to do the reverse -- they would be left with only the option of deleting the columns and the associated data. Here, the user can simply make the columns visible again to make the view match the desired export.
(4) interestingly default behavior is not uniform - LibreOffice ignores hidden columns by default on png/csv export and Google Spreadsheet does the opposite, can't test Excel

My suggestion is to expand on the export_headers prop like so:

    export_headers: PropType.oneOfType(
        PropTypes.oneOf(['none', 'ids', 'names', 'display']),
        PropTypes.exact({
            headers: PropTypes.oneOf(['none', 'ids', 'names', 'display']),
            columns: Proptypes.oneOf(['all', 'visible'])
        })
    ),

As we did elsewhere, leaving the basic usage simple but presenting a more complex prop for tweaking / advance usage.

This should be simple enough to support, and could be folded in with the fix.

@alexcjohnson
Copy link
Collaborator

Whether to include all or just visible columns doesn't really fit under the prop export_headers IMO - I'd just make a new prop export_columns: Proptypes.oneOf(['all', 'visible'])

@Marc-Andre-Rivet
Copy link
Contributor Author

Fair enough. My attempts to minimize props were ill advised.

- merge from 'dev'
1384a72 
- new export_columns prop
- change export logic to export all columns if desired
- additional typing
- typo fix
- changelog entry
@chriddyp chriddyp temporarily deployed to dash-table-review-pr-592 September 19, 2019 20:58 Inactive
Marc-Andre-Rivet
Marc-Andre-Rivet commented Sep 19, 2019
View reviewed changes
src/dash-table/components/Export/index.tsx
const { columns, export_columns, export_format, virtual_data, export_headers, visibleColumns, merge_duplicate_headers } = props;
const isFormatSupported = export_format === ExportFormat.Csv || export_format === ExportFormat.Xlsx;

const exportedColumns = export_columns === ExportColumns.Visible ? visibleColumns : columns;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use either visible columns or all columns depending on the prop setting

Marc-Andre-Rivet
Marc-Andre-Rivet commented Sep 19, 2019
View reviewed changes
src/dash-table/components/Export/index.tsx
import { createWorkbook, createHeadings, createWorksheet } from './utils';
import getHeaderRows from 'dash-table/derived/header/headerRows';

interface IExportButtonProps {
columns: Columns;
export_format: string;
export_columns: ExportColumns;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated string usage to enums

Marc-Andre-Rivet
Marc-Andre-Rivet commented Sep 19, 2019
View reviewed changes
src/dash-table/components/Export/utils.tsx

interface IMergeObject {
s: {r: number, c: number};
e: {r: number, c: number};
}

export function transformMultDimArray(array: (string | string[])[], maxLength: number): string[][] {
export function transformMultiDimArray(array: (string | string[])[], maxLength: number): string[][] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo

Marc-Andre-Rivet
Marc-Andre-Rivet commented Sep 19, 2019
View reviewed changes
tests/cypress/tests/unit/exportUtils_tests.ts
{col2: 2, col3: 3}
{col2: 2, col4: 3},
{col2: 3, col4: 4},
{col2: 2, col4: 3}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

col3 became col4 above

@chriddyp chriddyp temporarily deployed to dash-table-review-pr-592 September 19, 2019 21:05 Inactive
alexcjohnson
alexcjohnson reviewed Sep 19, 2019
View reviewed changes
alexcjohnson
alexcjohnson approved these changes Sep 19, 2019
View reviewed changes
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Very nice. Just the one comment about the changelog, then 💃

@Marc-Andre-Rivet Marc-Andre-Rivet merged commit 5df7dce into dev Sep 20, 2019
@Marc-Andre-Rivet Marc-Andre-Rivet deleted the 546-export-hidden-columns branch September 20, 2019 01:26
shammamah-zz pushed a commit that referenced this pull request Sep 20, 2019
@Marc-Andre-Rivet
Issue 546 - Visible columns and data misalignment in export (#592)
e941cd1
@laoxu0212 laoxu0212 mentioned this pull request Sep 20, 2019
Closed
This was referenced Sep 30, 2019
Closed
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v1.1.0 datatable hidden_columns/exporting loses hidden headers when export_header='names'
3 participants
@Marc-Andre-Rivet @alexcjohnson @chriddyp