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/06/13 00:31:07 UTC
[GitHub] [incubator-superset] ktmud commented on a change in pull request #10043: feat: add more columns and icons to dataset listview
ktmud commented on a change in pull request #10043:
URL: https://github.com/apache/incubator-superset/pull/10043#discussion_r439680610
##########
File path: superset-frontend/src/components/IndeterminateCheckbox.tsx
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import styled from '@superset-ui/style';
+import { ReactComponent as CheckboxOnIcon } from 'images/icons/checkbox-on.svg';
+import { ReactComponent as CheckboxOffIcon } from 'images/icons/checkbox-off.svg';
+import { ReactComponent as CheckboxHalfIcon } from 'images/icons/checkbox-half.svg';
+
+interface Props {
Review comment:
Prefers `IndeterminateCheckboxProps`: https://github.com/apache-superset/superset-ui/pull/586
##########
File path: superset-frontend/src/components/IndeterminateCheckbox.tsx
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import styled from '@superset-ui/style';
+import { ReactComponent as CheckboxOnIcon } from 'images/icons/checkbox-on.svg';
+import { ReactComponent as CheckboxOffIcon } from 'images/icons/checkbox-off.svg';
+import { ReactComponent as CheckboxHalfIcon } from 'images/icons/checkbox-half.svg';
+
+interface Props {
+ indeterminate: boolean;
+ id: string;
+ checked: boolean;
+ onChange: React.EventHandler<React.SyntheticEvent<HTMLInputElement>>;
+ title?: string;
+}
+
+const CheckboxLabel = styled.label`
+ cursor: pointer;
+ margin-bottom: 0;
+`;
+
+const IndeterminateCheckbox = React.forwardRef(
+ (
+ { indeterminate, id, checked, onChange, title = '' }: Props,
+ ref: React.MutableRefObject<any>,
+ ) => {
+ const defaultRef = React.useRef<HTMLInputElement>();
+ const resolvedRef = ref || defaultRef;
+
+ React.useEffect(() => {
+ resolvedRef.current.indeterminate = indeterminate;
+ }, [resolvedRef, indeterminate]);
+
+ return (
+ <>
+ <CheckboxLabel htmlFor={id} title={title}>
+ {indeterminate && <CheckboxHalfIcon />}
+ {!indeterminate && checked && <CheckboxOnIcon />}
+ {!indeterminate && !checked && <CheckboxOffIcon />}
+ </CheckboxLabel>
Review comment:
I think it's valid HTML for `<label>` to wrap an `<input>` element, with which you can also spare `<>...</>` and the `htmlFor` attribute.
##########
File path: superset-frontend/src/components/IndeterminateCheckbox.tsx
##########
@@ -0,0 +1,71 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React from 'react';
+import styled from '@superset-ui/style';
+import { ReactComponent as CheckboxOnIcon } from 'images/icons/checkbox-on.svg';
+import { ReactComponent as CheckboxOffIcon } from 'images/icons/checkbox-off.svg';
+import { ReactComponent as CheckboxHalfIcon } from 'images/icons/checkbox-half.svg';
Review comment:
According to [its README](https://www.npmjs.com/package/@svgr/webpack), doesn't `@svgr/webpack` export the react component as default?
```jsx
import Star from './star.svg'
const App = () => (
<div>
<Star />
</div>
)
```
##########
File path: superset-frontend/src/components/ListView/TableCollection.tsx
##########
@@ -41,28 +44,30 @@ export default function TableCollection({
<thead>
{headerGroups.map(headerGroup => (
<tr {...headerGroup.getHeaderGroupProps()}>
- {headerGroup.headers.map(column =>
- column.hidden ? null : (
+ {headerGroup.headers.map(column => {
+ let sortIcon;
+ if (!column.isSorted) {
+ sortIcon = <SortIcon />;
+ } else if (column.isSorted && column.isSortedDesc) {
+ sortIcon = <SortDescIcon />;
+ } else if (column.isSorted && !column.isSortedDesc) {
Review comment:
Nit: `column.isSorted &&` is not needed for the two `else if`.
Or:
```jsx
let CurrentSortIcon = SortIcon;
if (column.isSorted) {
CurrentSortIcon = column.isSortedDesc ? SortDescIcon : SortAscIcon;
}
<span className="sort-icon"><CurrentSortIcon /></span>
```
##########
File path: superset-frontend/src/components/ListView/ListView.tsx
##########
@@ -51,12 +51,15 @@ interface Props {
const bulkSelectColumnConfig = {
Cell: ({ row }: any) => (
<div>
- <IndeterminateCheckbox {...row.getToggleRowSelectedProps()} />
+ <IndeterminateCheckbox {...row.getToggleRowSelectedProps()} id={row.id} />
Review comment:
Not the scope of this PR, but have we considered allowing "Shift + Click" to select/unselect all rows between previous and current selected rows? This is a pretty common user interaction expected from a data table with checkboxes.
Another convenient thing to add is to allow users to select a row by clicking anywhere on the row (except links and buttons, of course), instead of just the checkbox.
##########
File path: superset-frontend/src/views/datasetList/DatasetList.tsx
##########
@@ -141,44 +146,62 @@ class DatasetList extends React.PureComponent<Props, State> {
{
Cell: ({
row: {
- original: { explore_url: exploreUrl, table_name: datasetTitle },
+ original: { kind },
},
- }: any) => <a href={exploreUrl}>{datasetTitle}</a>,
- Header: t('Table'),
- accessor: 'table_name',
+ }: any) =>
+ kind === 'physical' ? <DatasetPhysicalIcon /> : <DatasetVirtualIcon />,
Review comment:
Will the user understand the difference between these two icons? Should we maybe add a tooltip?
----------------------------------------------------------------
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