etr2460 commented on a change in pull request #11854:
URL:
https://github.com/apache/incubator-superset/pull/11854#discussion_r534657770
##########
File path: superset-frontend/src/explore/components/DataTableControl.jsx
##########
@@ -0,0 +1,91 @@
+/**
Review comment:
this is a new file, can you please use TypeScript?
##########
File path: superset-frontend/src/explore/components/DataTableControl.jsx
##########
@@ -0,0 +1,91 @@
+/**
+ * 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, { useMemo } from 'react';
+import { styled, t } from '@superset-ui/core';
+import { FormControl } from 'react-bootstrap';
+import Button from '../../components/Button';
+import RowCountLabel from './RowCountLabel';
+import {
+ applyFormattingToTabularData,
+ prepareCopyToClipboardTabularData,
+} from '../../utils/common';
+import CopyToClipboard from '../../components/CopyToClipboard';
+
+const CopyButton = styled(Button)`
+ padding: ${({ theme }) => theme.gridUnit / 2}px
Review comment:
@rusackas do you think we should be doing division on `gridUnit`? I
thought the intention was for this to be the smallest unit of sizing in the app
##########
File path: superset-frontend/src/explore/components/ExploreChartPanel.jsx
##########
@@ -49,22 +52,124 @@ const propTypes = {
triggerRender: PropTypes.bool,
};
+export const EXPLORE_GUTTER_HEIGHT = 5;
+export const EXPLORE_GUTTER_MARGIN = 3;
+export const CHART_PANEL_PADDING = 30;
+
+const INITIAL_SIZES = [90, 10];
+const MIN_SIZES = [300, 50];
+
const Styles = styled.div`
background-color: ${({ theme }) => theme.colors.grayscale.light5};
height: 100%;
display: flex;
flex-direction: column;
align-items: stretch;
align-content: stretch;
+ overflow: hidden;
+
& > div:last-of-type {
flex-basis: 100%;
}
+
+ .gutter {
+ border-top: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ width: 3%;
Review comment:
should the gutter be a % of the total width? Seems like a constant size
would be better (both for folks with small and large width monitors)
##########
File path: superset-frontend/src/explore/components/DataTablesPane.jsx
##########
@@ -0,0 +1,201 @@
+/**
Review comment:
Since this is a new file, can you use TypeScript?
##########
File path: superset-frontend/src/explore/components/DataTableControl.jsx
##########
@@ -0,0 +1,91 @@
+/**
+ * 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, { useMemo } from 'react';
+import { styled, t } from '@superset-ui/core';
+import { FormControl } from 'react-bootstrap';
+import Button from '../../components/Button';
+import RowCountLabel from './RowCountLabel';
+import {
+ applyFormattingToTabularData,
+ prepareCopyToClipboardTabularData,
+} from '../../utils/common';
+import CopyToClipboard from '../../components/CopyToClipboard';
+
+const CopyButton = styled(Button)`
+ padding: ${({ theme }) => theme.gridUnit / 2}px
+ ${({ theme }) => theme.gridUnit * 2.5}px;
+ font-size: ${({ theme }) => theme.typography.sizes.s}px;
+
+ // needed to override button's first-of-type margin: 0
+ && {
+ margin: 0 ${({ theme }) => theme.gridUnit * 2}px;
+ }
+
+ i {
+ padding: 0;
+ }
+`;
+
+export const CopyToClipboardButton = ({ data }) => (
+ <CopyToClipboard
+ text={data ? prepareCopyToClipboardTabularData(data) : ''}
+ wrapped={false}
+ copyNode={
+ <CopyButton>
+ <i className="fa fa-clipboard" />
+ </CopyButton>
+ }
+ />
+);
+
+export const FilterInput = ({ filterText, onChangeHandler }) => (
+ <FormControl
+ placeholder={t('Search')}
+ bsSize="sm"
+ value={filterText}
+ onChange={onChangeHandler}
+ style={{ paddingBottom: '5px' }}
Review comment:
can we use `styled` to apply the css here (or at least get the constant
padding from the superset theme)?
##########
File path: superset-frontend/src/explore/components/DataTablesPane.jsx
##########
@@ -0,0 +1,201 @@
+/**
+ * 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, { useEffect, useState } from 'react';
+import PropTypes from 'prop-types';
+import { styled, t } from '@superset-ui/core';
+import {
+ CopyToClipboardButton,
+ FilterInput,
+ RowCount,
+ useFilteredTableData,
+ useTableColumns,
+} from './DataTableControl';
+import { Collapse } from '../../common/components';
+import Tabs from '../../common/components/Tabs';
+import Loading from '../../components/Loading';
+import TableView, { EmptyWrapperType } from '../../components/TableView';
+import { getChartDataRequest } from '../../chart/chartAction';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+const RESULT_TYPES = {
+ results: 'results',
+ samples: 'samples',
+};
+const NULLISH_RESULTS_STATE = {
+ [RESULT_TYPES.results]: null,
+ [RESULT_TYPES.samples]: null,
+};
+
+const TableControlsWrapper = styled.div`
+ display: flex;
+ align-items: center;
+
+ span {
+ flex-shrink: 0;
+ }
+`;
+
+const SouthPane = styled.div`
+ background-color: ${({ theme }) => theme.colors.grayscale.light5};
+ z-index: 1;
+`;
+
+const TabsWrapper = styled.div`
+ height: ${({ contentHeight }) => contentHeight}px;
+ overflow: hidden;
+
+ .table-condensed {
+ height: 100%;
+ overflow: auto;
+ }
+`;
+
+export const DataTablesPane = ({
+ queryFormData,
+ tableSectionHeight,
+ onCollapseChange,
+}) => {
+ const [data, setData] = useState(NULLISH_RESULTS_STATE);
+ const [isLoading, setIsLoading] = useState(NULLISH_RESULTS_STATE);
+ const [error, setError] = useState(NULLISH_RESULTS_STATE);
+ const [filterText, setFilterText] = useState('');
+ const [activeTabKey, setActiveTabKey] = useState(RESULT_TYPES.results);
+
+ useEffect(() => {
+ const getData = resultType => {
+ setIsLoading(prevIsLoading => ({ ...prevIsLoading, [resultType]: true
}));
+ return getChartDataRequest({
+ formData: queryFormData,
+ resultFormat: 'json',
+ resultType,
+ })
+ .then(response => {
+ // Currently displaying of only first query is supported
Review comment:
grammar nit: `Only displaying the first query is currently supported`
##########
File path: superset-frontend/src/explore/components/ExploreChartPanel.jsx
##########
@@ -49,22 +52,124 @@ const propTypes = {
triggerRender: PropTypes.bool,
};
+export const EXPLORE_GUTTER_HEIGHT = 5;
+export const EXPLORE_GUTTER_MARGIN = 3;
+export const CHART_PANEL_PADDING = 30;
+
+const INITIAL_SIZES = [90, 10];
+const MIN_SIZES = [300, 50];
+
const Styles = styled.div`
background-color: ${({ theme }) => theme.colors.grayscale.light5};
height: 100%;
display: flex;
flex-direction: column;
align-items: stretch;
align-content: stretch;
+ overflow: hidden;
+
& > div:last-of-type {
flex-basis: 100%;
}
+
+ .gutter {
+ border-top: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ border-bottom: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
+ width: 3%;
+ margin: ${EXPLORE_GUTTER_MARGIN}px 47%;
+ }
+
+ .gutter.gutter-vertical {
+ cursor: row-resize;
+ }
+
+ .ant-collapse {
+ height: 100%;
+ background-color: ${({ theme }) => theme.colors.grayscale.light5};
+ .ant-collapse-item {
+ height: 100%;
+ border: 0;
+ }
+ .ant-collapse-content,
+ .ant-collapse-content-box {
+ height: 100%;
+ }
+ .ant-collapse-header {
+ padding-top: 0;
+ padding-bottom: 0;
+ }
+ .ant-tabs {
+ height: 100%;
+ .ant-tabs-nav {
+ padding-left: ${({ theme }) => theme.gridUnit * 5}px;
+ margin: 0;
+ background-color: ${({ theme }) => theme.colors.grayscale.light5};
+ }
+ .ant-tabs-content-holder {
+ overflow: hidden;
+ .ant-tabs-content {
+ height: 100%;
+ }
+ }
+ }
+ }
`;
-class ExploreChartPanel extends React.PureComponent {
- renderChart() {
- const { chart } = this.props;
- const headerHeight = this.props.standalone ? 0 : 80;
+const ExploreChartPanel = props => {
+ const panelHeadingRef = useRef(null);
+ const [headerHeight, setHeaderHeight] = useState(props.standalone ? 0 : 50);
+ const [splitSizes, setSplitSizes] = useState(INITIAL_SIZES);
+
+ const calcSectionHeight = percent => {
+ const containerHeight = parseInt(props.height, 10) - headerHeight - 30;
+ return (
+ (containerHeight * percent) / 100 -
+ (EXPLORE_GUTTER_HEIGHT / 2 + EXPLORE_GUTTER_MARGIN)
+ );
+ };
+
+ const [chartSectionHeight, setChartSectionHeight] = useState(
+ calcSectionHeight(INITIAL_SIZES[0]) - CHART_PANEL_PADDING,
+ );
+ const [tableSectionHeight, setTableSectionHeight] = useState(
+ calcSectionHeight(INITIAL_SIZES[1]),
+ );
+
+ useEffect(() => {
+ const calcHeaderSize = debounce(() => {
+ setHeaderHeight(
+ props.standalone ? 0 : panelHeadingRef?.current?.offsetHeight,
+ );
+ }, 100);
+ calcHeaderSize();
+ document.addEventListener('resize', calcHeaderSize);
+ return () => document.removeEventListener('resize', calcHeaderSize);
+ }, [props.standalone]);
+
+ const recalcPanelSizes = ([northPercent, southPercent]) => {
+ setChartSectionHeight(
+ calcSectionHeight(northPercent) - CHART_PANEL_PADDING,
+ );
+ setTableSectionHeight(calcSectionHeight(southPercent));
+ };
+
+ const onCollapseChange = openPanelName => {
+ const defaultSouthPaneOpenHeightPercent = 40;
Review comment:
maybe make this a constant outside of the component?
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
