You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/15 04:43:32 UTC
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10298: feat(listviews): SIP-34 Bulk Select
etr2460 commented on a change in pull request #10298:
URL: https://github.com/apache/incubator-superset/pull/10298#discussion_r454785527
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -42,12 +44,52 @@ interface Props {
filters?: Filters;
bulkActions?: Array<{
key: string;
- name: React.ReactNode | string;
+ name: React.ReactNode;
onSelect: (rows: any[]) => any;
+ type?: 'primary' | 'secondary' | 'danger';
}>;
isSIP34FilterUIEnabled?: boolean;
+ bulkSelectEnabled?: boolean;
+ disableBulkSelect?: () => void;
+ renderBulkSelectCopy?: (selects: any[]) => React.ReactNode;
}
+const BulkSelectWrapper = styled(Alert)`
+ border-radius: 0;
+ margin-bottom: 0;
+ padding-top: 0;
+ padding-bottom: 0;
+ padding-right: 16px;
+ padding-right: 36px;
+ color: #3d3d3d;
+ background-color: ${({ theme }) => theme.colors.primary.light4};
+
+ .selectedCopy {
+ display: inline-block;
+ padding: 16px 0;
+ }
+
+ .deselect-all {
+ color: #1985a0;
+ margin-left: 16px;
+ }
+
+ .divider {
+ margin: -8px 0 -8px 16px;
+ width: 1px;
+ height: 32px;
+ background: rgba(0, 0, 0, 0.0001);
Review comment:
Is this color even visible?
##########
File path: superset-frontend/src/components/Button.tsx
##########
@@ -17,41 +17,75 @@
* under the License.
*/
import React from 'react';
-import PropTypes from 'prop-types';
import { kebabCase } from 'lodash';
import {
Button as BootstrapButton,
Tooltip,
OverlayTrigger,
} from 'react-bootstrap';
+import styled from '@superset-ui/style';
-const propTypes = {
- children: PropTypes.node,
- className: PropTypes.string,
- tooltip: PropTypes.node,
- placement: PropTypes.string,
- onClick: PropTypes.func,
- disabled: PropTypes.bool,
- bsSize: PropTypes.string,
- bsStyle: PropTypes.string,
- btnStyles: PropTypes.string,
-};
-const defaultProps = {
- bsSize: 'sm',
- placement: 'top',
-};
+export type OnClickHandler = React.MouseEventHandler<BootstrapButton>;
+
+export interface ButtonProps {
+ className?: string;
+ tooltip?: string;
+ placement?: string;
+ onClick?: OnClickHandler;
+ disabled?: boolean;
+ bsStyle?: string;
+ btnStyles?: string;
+ bsSize?: BootstrapButton.ButtonProps['bsSize'];
+ style?: BootstrapButton.ButtonProps['style'];
+}
const BUTTON_WRAPPER_STYLE = { display: 'inline-block', cursor: 'not-allowed' };
-export default function Button(props) {
- const buttonProps = { ...props };
+const SupersetButton = styled(BootstrapButton)`
Review comment:
How is this styling different from `/views/datasetList/Button`? Should these two components be joined together?
cc @lilykuang
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -155,6 +201,43 @@ const ListView: FunctionComponent<Props> = ({
)}
</div>
<div className="body">
+ {bulkSelectEnabled && (
+ <BulkSelectWrapper
+ data-test="bulk-select-controls"
+ bsStyle="info"
+ onDismiss={disableBulkSelect}
+ >
+ <div className="selectedCopy" data-test="bulk-select-copy">
+ {renderBulkSelectCopy(selectedFlatRows)}
+ </div>
+ {Boolean(selectedFlatRows.length) && (
+ <>
+ <span
+ data-test="bulk-select-deselect-all"
+ role="button"
+ tabIndex={0}
+ className="deselect-all"
+ onClick={() => toggleAllRowsSelected(false)}
+ >
+ {t('Deselect All')}
+ </span>
+ <div className="divider" />
+ {bulkActions.map(action => (
+ <Button
+ data-test="bulk-select-action"
+ key={action.key}
+ className={`supersetButton ${action.type}`}
Review comment:
should this use `cx`?
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -42,12 +44,52 @@ interface Props {
filters?: Filters;
bulkActions?: Array<{
key: string;
- name: React.ReactNode | string;
+ name: React.ReactNode;
onSelect: (rows: any[]) => any;
+ type?: 'primary' | 'secondary' | 'danger';
}>;
isSIP34FilterUIEnabled?: boolean;
+ bulkSelectEnabled?: boolean;
+ disableBulkSelect?: () => void;
+ renderBulkSelectCopy?: (selects: any[]) => React.ReactNode;
}
+const BulkSelectWrapper = styled(Alert)`
+ border-radius: 0;
+ margin-bottom: 0;
+ padding-top: 0;
+ padding-bottom: 0;
+ padding-right: 16px;
+ padding-right: 36px;
Review comment:
probably only one of these should exist
##########
File path: superset-frontend/src/components/Menu/SubMenu.tsx
##########
@@ -16,40 +16,29 @@
* specific language governing permissions and limitations
* under the License.
*/
-import React, { useState } from 'react';
+import React from 'react';
import styled from '@superset-ui/style';
-import DatasetModal from 'src/views/datasetList/DatasetModal';
-import { Button, Nav, Navbar, MenuItem } from 'react-bootstrap';
+import { Nav, Navbar, MenuItem } from 'react-bootstrap';
+import Button, { OnClickHandler } from 'src/components/Button';
const StyledHeader = styled.header`
margin-top: -20px;
.navbar-header .navbar-brand {
font-weight: ${({ theme }) => theme.typography.weights.bold};
}
-
.navbar-right {
- .btn-default {
- background-color: ${({ theme }) => theme.colors.primary.base};
- border-radius: 4px;
- border: none;
- color: ${({ theme }) => theme.colors.secondary.light5};
- font-size: ${({ theme }) => theme.typography.sizes.s};
- font-weight: ${({ theme }) => theme.typography.weights.bold};
- margin: 8px 43px;
- padding: 8px 51px 8px 43px;
- text-transform: uppercase;
- i {
- padding: 4px ${({ theme }) => theme.typography.sizes.xs};
- }
+ .supersetButton {
+ margin: ${({ theme }) => theme.gridUnit * 2}px
+ ${({ theme }) => theme.gridUnit * 4}px
+ ${({ theme }) => theme.gridUnit * 2}px 0;
Review comment:
this is kinda tough to read... maybe this is easier?:
```
${({theme}) => `${theme.gridUnit * 2}px ${theme.gridUnit * 4}px ${theme.gridUnit * 2}px 0`}
```
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -532,50 +567,64 @@ const DatasetList: FunctionComponent<DatasetListProps> = ({
onConfirm={handleBulkDatasetDelete}
>
{confirmDelete => {
- const bulkActions = [];
- if (canDelete()) {
- bulkActions.push({
- key: 'delete',
- name: (
- <>
- <i className="fa fa-trash" /> {t('Delete')}
- </>
- ),
- onSelect: confirmDelete,
- });
- }
+ const bulkActions: ListViewProps['bulkActions'] = canDelete
+ ? [
+ {
+ key: 'delete',
+ name: t('Delete'),
+ onSelect: confirmDelete,
+ type: 'danger',
+ },
+ ]
+ : [];
+
return (
- <>
- {datasetCurrentlyDeleting && (
- <DeleteModal
- description={t(
- `The dataset ${datasetCurrentlyDeleting.table_name} is linked to
- ${datasetCurrentlyDeleting.chart_count} charts that appear on
- ${datasetCurrentlyDeleting.dashboard_count} dashboards.
- Are you sure you want to continue? Deleting the dataset will break
- those objects.`,
- )}
- onConfirm={() =>
- handleDatasetDelete(datasetCurrentlyDeleting)
- }
- onHide={closeDatasetDeleteModal}
- open
- title={t('Delete Dataset?')}
- />
- )}
- <ListView
- className="dataset-list-view"
- columns={columns}
- data={datasets}
- count={datasetCount}
- pageSize={PAGE_SIZE}
- fetchData={fetchData}
- loading={loading}
- initialSort={initialSort}
- filters={currentFilters}
- bulkActions={bulkActions}
- />
- </>
+ <ListView
+ className="dataset-list-view"
+ columns={columns}
+ data={datasets}
+ count={datasetCount}
+ pageSize={PAGE_SIZE}
+ fetchData={fetchData}
+ loading={loading}
+ initialSort={initialSort}
+ filters={currentFilters}
+ bulkActions={bulkActions}
+ bulkSelectEnabled={bulkSelectEnabled}
+ disableBulkSelect={() => setBulkSelectEnabled(false)}
+ renderBulkSelectCopy={selected => {
+ const { virtualCount, physicalCount } = selected.reduce(
+ (acc, e) => {
+ if (e.original.kind === 'physical') acc.physicalCount += 1;
+ else if (e.original.kind === 'virtual')
+ acc.virtualCount += 1;
+ return acc;
+ },
+ { virtualCount: 0, physicalCount: 0 },
+ );
+
+ if (!selected.length) return t('0 Selected');
+ else if (virtualCount && !physicalCount)
+ return t(
+ '%s Selected (Virtual)',
+ selected.length,
+ virtualCount,
+ );
+ else if (physicalCount && !virtualCount)
+ return t(
+ '%s Selected (Physical)',
+ selected.length,
+ physicalCount,
+ );
Review comment:
can you make this use normal if/else braces? I'm surprised that prettier didn't actually catch this
##########
File path: superset-frontend/spec/helpers/waitForComponentToPaint.js
##########
@@ -0,0 +1,30 @@
+/**
Review comment:
as this is a new file, can it be in typescript?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org