From d62aa96354781052fa4b7ab9921b40b91f43bb5e Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 27 Apr 2022 11:35:36 +0300 Subject: [PATCH 1/5] chore: fix explore pills --- .../src/components/AlteredSliceTag/index.jsx | 16 ++++++++++++---- .../src/components/CachedLabel/index.tsx | 2 +- .../src/components/Label/Label.stories.tsx | 3 ++- .../src/components/Label/index.tsx | 17 ++++++++++++++--- .../components/DataTableControl/index.tsx | 8 +------- .../RowCountLabel/RowCountLabel.stories.tsx | 13 ++++++------- .../explore/components/RowCountLabel/index.tsx | 13 +++++++------ 7 files changed, 43 insertions(+), 29 deletions(-) diff --git a/superset-frontend/src/components/AlteredSliceTag/index.jsx b/superset-frontend/src/components/AlteredSliceTag/index.jsx index e4a0d1bdebea7..372cf8e60ce7f 100644 --- a/superset-frontend/src/components/AlteredSliceTag/index.jsx +++ b/superset-frontend/src/components/AlteredSliceTag/index.jsx @@ -19,7 +19,7 @@ import React from 'react'; import PropTypes from 'prop-types'; import { isEqual, isEmpty } from 'lodash'; -import { t } from '@superset-ui/core'; +import { styled, t } from '@superset-ui/core'; import { sanitizeFormData } from 'src/explore/exploreUtils/formData'; import getControlsForVizType from 'src/utils/getControlsForVizType'; import { safeStringify } from 'src/utils/safeStringify'; @@ -32,6 +32,16 @@ const propTypes = { currentFormData: PropTypes.object.isRequired, }; +const StyledLabel = styled.span` + ${({ theme }) => ` + font-size: ${theme.typography.sizes.s}px; + background-color: ${theme.colors.alert.base}; + + &: hover { + background-color: ${theme.colors.alert.dark1}; + };`} +`; + function alterForComparison(value) { // Considering `[]`, `{}`, `null` and `undefined` as identical // for this purpose @@ -183,9 +193,7 @@ export default class AlteredSliceTag extends React.Component { renderTriggerNode() { return ( - - {t('Altered')} - + {t('Altered')} ); } diff --git a/superset-frontend/src/components/CachedLabel/index.tsx b/superset-frontend/src/components/CachedLabel/index.tsx index a401ab1e0e57c..a6c74cb8eb62d 100644 --- a/superset-frontend/src/components/CachedLabel/index.tsx +++ b/superset-frontend/src/components/CachedLabel/index.tsx @@ -48,7 +48,7 @@ const CacheLabel: React.FC = ({ onMouseOver={() => setHovered(true)} onMouseOut={() => setHovered(false)} > - {t('cached')} + {t('Cached')} ); diff --git a/superset-frontend/src/components/Label/Label.stories.tsx b/superset-frontend/src/components/Label/Label.stories.tsx index 07fc7c09fa744..90be09651fa96 100644 --- a/superset-frontend/src/components/Label/Label.stories.tsx +++ b/superset-frontend/src/components/Label/Label.stories.tsx @@ -26,7 +26,8 @@ export default { excludeStories: 'options', }; -export const options = [ +export const options: Type[] = [ + 'alert', 'default', 'info', 'success', diff --git a/superset-frontend/src/components/Label/index.tsx b/superset-frontend/src/components/Label/index.tsx index e66437da7d8a5..508d83f961c2f 100644 --- a/superset-frontend/src/components/Label/index.tsx +++ b/superset-frontend/src/components/Label/index.tsx @@ -23,6 +23,7 @@ import { useTheme } from '@superset-ui/core'; export type OnClickHandler = React.MouseEventHandler; export type Type = + | 'alert' | 'success' | 'warning' | 'danger' @@ -45,8 +46,16 @@ export default function Label(props: LabelProps) { const theme = useTheme(); const { colors, transitionTiming } = theme; const { type, onClick, children, ...rest } = props; - const { primary, secondary, grayscale, success, warning, error, info } = - colors; + const { + alert, + primary, + secondary, + grayscale, + success, + warning, + error, + info, + } = colors; let backgroundColor = grayscale.light3; let backgroundColorHover = onClick ? primary.light2 : grayscale.light3; @@ -58,7 +67,9 @@ export default function Label(props: LabelProps) { color = grayscale.light4; let baseColor; - if (type === 'success') { + if (type === 'alert') { + baseColor = alert; + } else if (type === 'success') { baseColor = success; } else if (type === 'warning') { baseColor = warning; diff --git a/superset-frontend/src/explore/components/DataTableControl/index.tsx b/superset-frontend/src/explore/components/DataTableControl/index.tsx index 7a25c374bd8ac..791f5f8ff3898 100644 --- a/superset-frontend/src/explore/components/DataTableControl/index.tsx +++ b/superset-frontend/src/explore/components/DataTableControl/index.tsx @@ -127,13 +127,7 @@ export const RowCount = ({ }: { data?: Record[]; loading: boolean; -}) => ( - -); +}) => ; enum FormatPickerValue { Formatted, diff --git a/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.stories.tsx b/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.stories.tsx index 716830f9ca32b..558f14f308c28 100644 --- a/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.stories.tsx +++ b/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.stories.tsx @@ -17,17 +17,21 @@ * under the License. */ import React from 'react'; -import RowCountLabel from '.'; +import RowCountLabel, { RowCountLabelProps } from '.'; export default { title: 'RowCountLabel', component: RowCountLabel, }; -const options = { +const options: { [key in string]: RowCountLabelProps } = { loading: { loading: true, }, + single: { + rowcount: 1, + limit: 100, + }, full: { rowcount: 100, limit: 100, @@ -36,10 +40,6 @@ const options = { rowcount: 50, limit: 100, }, - suffix: { - rowcount: 1, - suffix: 'suffix', - }, }; export const RowCountLabelGallery = () => ( @@ -51,7 +51,6 @@ export const RowCountLabelGallery = () => ( loading={options[name].loading} rowcount={options[name].rowcount} limit={options[name].limit} - suffix={options[name].suffix} /> ))} diff --git a/superset-frontend/src/explore/components/RowCountLabel/index.tsx b/superset-frontend/src/explore/components/RowCountLabel/index.tsx index c612856817c21..607ea0004f172 100644 --- a/superset-frontend/src/explore/components/RowCountLabel/index.tsx +++ b/superset-frontend/src/explore/components/RowCountLabel/index.tsx @@ -17,25 +17,24 @@ * under the License. */ import React from 'react'; -import { getNumberFormatter, t } from '@superset-ui/core'; +import { getNumberFormatter, t, tn } from '@superset-ui/core'; import Label from 'src/components/Label'; import { Tooltip } from 'src/components/Tooltip'; type RowCountLabelProps = { - rowcount: number; + rowcount?: number; limit?: number; - suffix?: string; loading?: boolean; }; export default function RowCountLabel(props: RowCountLabelProps) { - const { rowcount = 0, limit, suffix = t('rows'), loading } = props; + const { rowcount = 0, limit, loading } = props; const limitReached = rowcount === limit; const type = limitReached || (rowcount === 0 && !loading) ? 'danger' : 'default'; const formattedRowCount = getNumberFormatter()(rowcount); - const tooltip = ( + const tooltip = (limitReached || loading) && ( {limitReached &&
{t('Limit reached')}
} {loading ? 'Loading' : rowcount} @@ -44,7 +43,9 @@ export default function RowCountLabel(props: RowCountLabelProps) { return ( ); From 801100df6c91a78aaa126b0a9124e32da42485f6 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 27 Apr 2022 13:54:36 +0300 Subject: [PATCH 2/5] fix tests --- .../integration/explore/control.test.ts | 4 +- .../DataTableControl/RowCount.test.tsx | 9 ++- .../DataTablesPane/DataTablesPane.test.tsx | 8 +-- .../RowCountLabel/RowCountLabel.test.jsx | 50 ---------------- .../RowCountLabel/RowCountLabel.test.tsx | 57 +++++++++++++++++++ .../components/RowCountLabel/index.tsx | 30 ++++++---- 6 files changed, 88 insertions(+), 70 deletions(-) delete mode 100644 superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.jsx create mode 100644 superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx diff --git a/superset-frontend/cypress-base/cypress/integration/explore/control.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/control.test.ts index 97dfd2945aaf5..e2aec6b7b9873 100644 --- a/superset-frontend/cypress-base/cypress/integration/explore/control.test.ts +++ b/superset-frontend/cypress-base/cypress/integration/explore/control.test.ts @@ -122,12 +122,12 @@ describe('Test datatable', () => { }); it('Data Pane opens and loads results', () => { cy.contains('Results').click(); - cy.get('[data-test="row-count-label"]').contains('26 rows retrieved'); + cy.get('[data-test="row-count-label"]').contains('26 rows'); cy.get('.ant-empty-description').should('not.exist'); }); it('Datapane loads view samples', () => { cy.contains('Samples').click(); - cy.get('[data-test="row-count-label"]').contains('1k rows retrieved'); + cy.get('[data-test="row-count-label"]').contains('1k rows'); cy.get('.ant-empty-description').should('not.exist'); }); }); diff --git a/superset-frontend/src/explore/components/DataTableControl/RowCount.test.tsx b/superset-frontend/src/explore/components/DataTableControl/RowCount.test.tsx index be3f0e980c2b8..6ebbcef77b3f7 100644 --- a/superset-frontend/src/explore/components/DataTableControl/RowCount.test.tsx +++ b/superset-frontend/src/explore/components/DataTableControl/RowCount.test.tsx @@ -20,9 +20,14 @@ import React from 'react'; import { render, screen } from 'spec/helpers/testing-library'; import { RowCount } from '.'; -test('Render a RowCount', () => { +test('Render a RowCount with a single row', () => { + render(); + expect(screen.getByText('1 row')).toBeInTheDocument(); +}); + +test('Render a RowCount with multiple rows', () => { render(); - expect(screen.getByText('3 rows retrieved')).toBeInTheDocument(); + expect(screen.getByText('3 rows')).toBeInTheDocument(); }); test('Render a RowCount on loading', () => { diff --git a/superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.test.tsx b/superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.test.tsx index 786150449ee20..04869f5f10022 100644 --- a/superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.test.tsx +++ b/superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.test.tsx @@ -103,7 +103,7 @@ describe('DataTablesPane', () => { useRedux: true, }); userEvent.click(screen.getByText('Results')); - expect(await screen.findByText('0 rows retrieved')).toBeVisible(); + expect(await screen.findByText('0 rows')).toBeVisible(); expect(await screen.findByLabelText('Collapse data panel')).toBeVisible(); localStorage.clear(); }); @@ -114,7 +114,7 @@ describe('DataTablesPane', () => { useRedux: true, }); userEvent.click(screen.getByText('Samples')); - expect(await screen.findByText('0 rows retrieved')).toBeVisible(); + expect(await screen.findByText('0 rows')).toBeVisible(); expect(await screen.findByLabelText('Collapse data panel')).toBeVisible(); }); @@ -158,7 +158,7 @@ describe('DataTablesPane', () => { }, ); userEvent.click(screen.getByText('Results')); - expect(await screen.findByText('1 rows retrieved')).toBeVisible(); + expect(await screen.findByText('1 row')).toBeVisible(); userEvent.click(screen.getByLabelText('Copy')); expect(copyToClipboardSpy).toHaveBeenCalledWith( @@ -210,7 +210,7 @@ describe('DataTablesPane', () => { }, ); userEvent.click(screen.getByText('Results')); - expect(await screen.findByText('2 rows retrieved')).toBeVisible(); + expect(await screen.findByText('2 rows')).toBeVisible(); expect(screen.getByText('Action')).toBeVisible(); expect(screen.getByText('Horror')).toBeVisible(); diff --git a/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.jsx b/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.jsx deleted file mode 100644 index 452f68a745478..0000000000000 --- a/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.jsx +++ /dev/null @@ -1,50 +0,0 @@ -/** - * 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 { shallow } from 'enzyme'; - -import Label from 'src/components/Label'; -import { Tooltip } from 'src/components/Tooltip'; -import RowCountLabel from '.'; - -describe('RowCountLabel', () => { - const defaultProps = { - rowcount: 51, - limit: 100, - }; - - it('is valid', () => { - expect(React.isValidElement()).toBe( - true, - ); - }); - it('renders a Label and a Tooltip', () => { - const wrapper = shallow(); - expect(wrapper.find(Label)).toExist(); - expect(wrapper.find(Tooltip)).toExist(); - }); - it('renders a danger when limit is reached', () => { - const props = { - rowcount: 100, - limit: 100, - }; - const wrapper = shallow(); - expect(wrapper.find(Label).first().props().type).toBe('danger'); - }); -}); diff --git a/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx b/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx new file mode 100644 index 0000000000000..d39d7ea71b149 --- /dev/null +++ b/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx @@ -0,0 +1,57 @@ +/** + * 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 { render, screen } from 'spec/helpers/testing-library'; +import userEvent from '@testing-library/user-event'; + +import RowCountLabel from '.'; + +test('RowCountLabel renders singular result', () => { + render(); + const expectedText = '1 row'; + expect(screen.getByText(expectedText)).toBeInTheDocument(); + userEvent.hover(screen.getByText(expectedText)); + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument(); +}); + +test('RowCountLabel renders plural result', () => { + render(); + const expectedText = '2 rows'; + expect(screen.getByText(expectedText)).toBeInTheDocument(); + userEvent.hover(screen.getByText(expectedText)); + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument(); +}); + +test('RowCountLabel renders limit with danger and tooltip', async () => { + render(); + const expectedText = '100 rows'; + expect(screen.getByText(expectedText)).toBeInTheDocument(); + userEvent.hover(screen.getByText(expectedText)); + const tooltip = await screen.findByRole('tooltip'); + expect(tooltip).toHaveTextContent('Limit reached'); + expect(tooltip).toHaveStyle('background: rgba(0, 0, 0, 0.902);'); +}); + +test('RowCountLabel renders loading', () => { + render(); + const expectedText = 'Loading...'; + expect(screen.getByText(expectedText)).toBeInTheDocument(); + userEvent.hover(screen.getByText(expectedText)); + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument(); +}); diff --git a/superset-frontend/src/explore/components/RowCountLabel/index.tsx b/superset-frontend/src/explore/components/RowCountLabel/index.tsx index 607ea0004f172..f7e48e726df15 100644 --- a/superset-frontend/src/explore/components/RowCountLabel/index.tsx +++ b/superset-frontend/src/explore/components/RowCountLabel/index.tsx @@ -34,20 +34,26 @@ export default function RowCountLabel(props: RowCountLabelProps) { const type = limitReached || (rowcount === 0 && !loading) ? 'danger' : 'default'; const formattedRowCount = getNumberFormatter()(rowcount); - const tooltip = (limitReached || loading) && ( - - {limitReached &&
{t('Limit reached')}
} - {loading ? 'Loading' : rowcount} -
+ const label = ( + ); - return ( - - + return limitReached ? ( + +
{t('Limit reached')}
+
+ } + > + {label} + ) : ( + label ); } From ee1e723b4825fb1856a7a5e99dee79edadf70d1d Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 27 Apr 2022 16:20:55 +0300 Subject: [PATCH 3/5] address comments --- superset-frontend/src/components/AlteredSliceTag/index.jsx | 3 ++- .../src/explore/components/RowCountLabel/index.tsx | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/superset-frontend/src/components/AlteredSliceTag/index.jsx b/superset-frontend/src/components/AlteredSliceTag/index.jsx index 372cf8e60ce7f..66e2fadc54ba7 100644 --- a/superset-frontend/src/components/AlteredSliceTag/index.jsx +++ b/superset-frontend/src/components/AlteredSliceTag/index.jsx @@ -39,7 +39,8 @@ const StyledLabel = styled.span` &: hover { background-color: ${theme.colors.alert.dark1}; - };`} + } + `} `; function alterForComparison(value) { diff --git a/superset-frontend/src/explore/components/RowCountLabel/index.tsx b/superset-frontend/src/explore/components/RowCountLabel/index.tsx index f7e48e726df15..f29db5acd7d2d 100644 --- a/superset-frontend/src/explore/components/RowCountLabel/index.tsx +++ b/superset-frontend/src/explore/components/RowCountLabel/index.tsx @@ -37,7 +37,7 @@ export default function RowCountLabel(props: RowCountLabelProps) { const label = ( ); From 79eaf0bf1949ee107461144d1bbb497c9889ebba Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Wed, 27 Apr 2022 21:11:53 +0300 Subject: [PATCH 4/5] add test and remove redundant div --- .../components/RowCountLabel/RowCountLabel.test.tsx | 8 ++++++++ .../src/explore/components/RowCountLabel/index.tsx | 9 +-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx b/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx index d39d7ea71b149..2e7a44c15e159 100644 --- a/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx +++ b/superset-frontend/src/explore/components/RowCountLabel/RowCountLabel.test.tsx @@ -38,6 +38,14 @@ test('RowCountLabel renders plural result', () => { expect(screen.queryByRole('tooltip')).not.toBeInTheDocument(); }); +test('RowCountLabel renders formatted result', () => { + render(); + const expectedText = '1k rows'; + expect(screen.getByText(expectedText)).toBeInTheDocument(); + userEvent.hover(screen.getByText(expectedText)); + expect(screen.queryByRole('tooltip')).not.toBeInTheDocument(); +}); + test('RowCountLabel renders limit with danger and tooltip', async () => { render(); const expectedText = '100 rows'; diff --git a/superset-frontend/src/explore/components/RowCountLabel/index.tsx b/superset-frontend/src/explore/components/RowCountLabel/index.tsx index f29db5acd7d2d..3597e49724752 100644 --- a/superset-frontend/src/explore/components/RowCountLabel/index.tsx +++ b/superset-frontend/src/explore/components/RowCountLabel/index.tsx @@ -42,14 +42,7 @@ export default function RowCountLabel(props: RowCountLabelProps) { ); return limitReached ? ( - -
{t('Limit reached')}
- - } - > + {t('Limit reached')}}> {label} ) : ( From a57dfd11a2d6d17d27c6a378105c9c471c5bbdb0 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt Date: Thu, 28 Apr 2022 09:08:12 +0300 Subject: [PATCH 5/5] switch to dark text --- superset-frontend/src/components/AlteredSliceTag/index.jsx | 1 + superset-frontend/src/components/Label/Label.stories.tsx | 2 +- superset-frontend/src/components/Label/index.tsx | 5 +++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/components/AlteredSliceTag/index.jsx b/superset-frontend/src/components/AlteredSliceTag/index.jsx index 66e2fadc54ba7..2faa62c8908db 100644 --- a/superset-frontend/src/components/AlteredSliceTag/index.jsx +++ b/superset-frontend/src/components/AlteredSliceTag/index.jsx @@ -35,6 +35,7 @@ const propTypes = { const StyledLabel = styled.span` ${({ theme }) => ` font-size: ${theme.typography.sizes.s}px; + color: ${theme.colors.grayscale.dark1}; background-color: ${theme.colors.alert.base}; &: hover { diff --git a/superset-frontend/src/components/Label/Label.stories.tsx b/superset-frontend/src/components/Label/Label.stories.tsx index 90be09651fa96..9363cc80d33e9 100644 --- a/superset-frontend/src/components/Label/Label.stories.tsx +++ b/superset-frontend/src/components/Label/Label.stories.tsx @@ -27,8 +27,8 @@ export default { }; export const options: Type[] = [ - 'alert', 'default', + 'alert', 'info', 'success', 'warning', diff --git a/superset-frontend/src/components/Label/index.tsx b/superset-frontend/src/components/Label/index.tsx index 508d83f961c2f..eb2aff41d0d38 100644 --- a/superset-frontend/src/components/Label/index.tsx +++ b/superset-frontend/src/components/Label/index.tsx @@ -45,7 +45,7 @@ export interface LabelProps extends React.HTMLAttributes { export default function Label(props: LabelProps) { const theme = useTheme(); const { colors, transitionTiming } = theme; - const { type, onClick, children, ...rest } = props; + const { type = 'default', onClick, children, ...rest } = props; const { alert, primary, @@ -63,11 +63,12 @@ export default function Label(props: LabelProps) { let borderColorHover = onClick ? primary.light1 : 'transparent'; let color = grayscale.dark1; - if (type && type !== 'default') { + if (type !== 'default') { color = grayscale.light4; let baseColor; if (type === 'alert') { + color = grayscale.dark1; baseColor = alert; } else if (type === 'success') { baseColor = success;