AcmTable csv export button addition #3643
Conversation
Randy424
force-pushed
the
ACM-12099-acmtable-csv-export
branch
2 times, most recently
from
c6d6aac
to
00640d0
Compare
|
/hold |
frontend/package.json
Outdated
| @@ -95,6 +95,7 @@ | |||
| "uuid": "8.3.2", | |||
| "validator": "13.7.0", | |||
| "whatwg-fetch": "3.6.2", | |||
| "xlsx": "https://cdn.sheetjs.com/xlsx-0.20.2/xlsx-0.20.2.tgz", | |||
There was a problem hiding this comment.
I do not think this will work in our downstream builds because Cachito probably does not cache this package. They have not really provided a good explanation of why their package is not being published on npm.
SheetJS/sheetjs#2667
This package does a lot more than we need, and while I'm usually in favour of using a library, since we already have CSV export functionality in our UI that doesn't depend on a package, I think we should share that code instead.
| @@ -23,6 +23,14 @@ import { IAcmTableColumn } from './AcmTable' | |||
| import { useTranslation } from '../../lib/acm-i18next' | |||
| import ColumnsIcon from '@patternfly/react-icons/dist/js/icons/columns-icon' | |||
|
|
|||
| type PatternFlySpacingSetting = { | |||
There was a problem hiding this comment.
Don't duplicate type definitions when possible. We can just refer to ToolbarItemProps['spacer']. However, I have other suggestions that would avoid needing to add a prop for this at all.
| @@ -50,7 +60,7 @@ export function AcmManageColumn<T>({ | |||
| } | |||
|
|
|||
| return ( | |||
| <ToolbarItem> | |||
| <ToolbarItem spacer={toolbarItemSpacerSetting}> | |||
There was a problem hiding this comment.
I don't think the <ToolbarItem> component should be included in this component. I would move it up into the table. This component shouldn't be restricted to only appearing in a toolbar.
| @@ -1162,8 +1217,45 @@ export function AcmTable<T>(props: AcmTableProps<T>) { | |||
| <AcmManageColumn<T> | |||
| {...{ selectedColIds, setSelectedColIds, requiredColIds, defaultColIds, setColOrderIds, colOrderIds }} | |||
| allCols={columns.filter((col) => !col.isActionCol)} | |||
| toolbarItemSpacerSetting={showExportButton ? { default: 'spacerNone' } : undefined} | |||
There was a problem hiding this comment.
I think a more elegant solution than conditionally adding space to the right of this item would be to put the managed columns button and export dropdown in a group of their own:
<ToolbarGroup spaceItems={{ default: 'spaceItemsNone' }}>
There was a problem hiding this comment.
Excellent solution, thank you.
| isPlain | ||
| dropdownItems={[ | ||
| <DropdownItem | ||
| style={{ width: '10rem' }} |
There was a problem hiding this comment.
What is this for? Seems to add space on the right of the "Export as CSV" text.
There was a problem hiding this comment.
It was for just that...we don't necessarily need it, so I've removed it but it was mostly to agree with the stylization happening on the search page export dropdown.
Randy424
force-pushed
the
ACM-12099-acmtable-csv-export
branch
from
78497d6
to
00640d0
Compare
Randy424
force-pushed
the
ACM-12099-acmtable-csv-export
branch
3 times, most recently
from
2f7b4f8
to
88f4e66
Compare
|
/retest-required |
|
Running into a test failure on this component: src/routes/Infrastructure/Clusters/ManagedClusters/components/DownloadConfigurationDropdown.test.tsx ...It's reproducible locally, but I don't yet see how we have caused this issue. |
|
Looks like it's related to my use of the createDownloadFile() from the utils library: frontend/src/resources/utils/utils.ts |
Randy424
force-pushed
the
ACM-12099-acmtable-csv-export
branch
6 times, most recently
from
87fb2f3
to
455634f
Compare
|
/retest |
Signed-off-by: Randy Bruno Piverger <[email protected]>
Randy424
force-pushed
the
ACM-12099-acmtable-csv-export
branch
from
455634f
to
6909743
Compare
|
I think I fixed the issue, but the pipeline machine pools are shutting down? |
|
/retest-required |
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: KevinFCormier, Randy424 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
Regarding: https://issues.redhat.com/browse/ACM-12099?filter=-1
PR 1/7
Added exportContent prop to AcmTable, enabling the component to consume a callback that returns the column value's export string.
When used (see subsequent PR's for the implementation of this feature) the export button appears at the left end of the toolbar after the Manage columns button.