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/05/14 23:04:45 UTC
[GitHub] [incubator-superset] etr2460 commented on a change in pull request #9628: feat: upgrade react-select and make multi-select sortable
etr2460 commented on a change in pull request #9628:
URL: https://github.com/apache/incubator-superset/pull/9628#discussion_r424716562
##########
File path: superset-frontend/src/components/ColumnOption.jsx
##########
@@ -42,28 +42,28 @@ export default function ColumnOption({ column, showType }) {
}
return (
- <span>
+ <div className="column-option">
Review comment:
Why is this a div instead of a span now? I don't see anything in the css that would make this inline
##########
File path: superset-frontend/src/components/AsyncSelect.jsx
##########
@@ -18,7 +18,8 @@
*/
import React from 'react';
import PropTypes from 'prop-types';
-import Select from 'react-select';
+// TODO: refactor this with `import { AsyncSelect } from src/components/Select`
Review comment:
Does this mean this component is no longer actually Async?
##########
File path: superset-frontend/src/components/Select/styles.tsx
##########
@@ -0,0 +1,299 @@
+/**
+ * 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, { CSSProperties } from 'react';
+import { css, SerializedStyles } from '@emotion/core';
+import { supersetTheme } from '@superset-ui/style';
+import {
+ Styles,
+ Theme,
+ SelectComponentsConfig,
+ components as defaultComponents,
+} from 'react-select';
+import { colors as reactSelectColros } from 'react-select/src/theme';
+import { supersetColors } from 'src/components/styles';
+
+export const DEFAULT_CLASS_NAME = 'Select';
+export const DEFAULT_CLASS_NAME_PREFIX = 'Select';
+
+type RecursivePartial<T> = {
+ [P in keyof T]?: RecursivePartial<T[P]>;
+};
+
+export type ThemeConfig = {
+ borderRadius: number;
+ // z-index for menu dropdown
+ // (the same as `@z-index-above-dashboard-charts + 1` in variables.less)
+ zIndex: number;
+ colors: {
+ // add known colors
+ [key in keyof typeof reactSelectColros]: string;
+ } &
+ {
+ [key in keyof typeof supersetColors]: string;
+ } & {
+ [key: string]: string; // any other colors
+ };
+ spacing: Theme['spacing'] & {
+ // line height and font size must be pixels for easier computation
+ // of option item height in WindowedMenuList
+ lineHeight: number;
+ fontSize: number;
+ // other relative size must be string
+ minWidth: string;
+ };
+};
+
+export type PartialThemeConfig = RecursivePartial<ThemeConfig>;
+
+export const DEFAULT_THEME: PartialThemeConfig = {
+ borderRadius: parseInt(supersetTheme.borderRadius, 10),
+ zIndex: 11,
Review comment:
we have zIndex constants defined already, can we reuse them here? Or move them into someplace useable by both LESS and Styled Components
https://github.com/apache/incubator-superset/blob/291306392443a5a0d0e2ee0cc4a95d37c56d4589/superset-frontend/stylesheets/less/cosmo/variables.less#L270
##########
File path: superset-frontend/src/explore/AdhocFilter.js
##########
@@ -94,7 +91,7 @@ export default class AdhocFilter {
this.comparator = null;
}
this.isExtra = !!adhocFilter.isExtra;
- this.fromFormData = !!adhocFilter.filterOptionName;
+ this.isNew = adhocFilter.isNew || false;
Review comment:
why not `!!adhocFilter.isNew`?
##########
File path: superset-frontend/src/components/Select/WindowedSelect/WindowedMenuList.tsx
##########
@@ -0,0 +1,155 @@
+/**
+ * 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, {
+ useRef,
+ useEffect,
+ Component,
+ FunctionComponent,
+ RefObject,
+} from 'react';
+import {
+ ListChildComponentProps,
+ FixedSizeList as WindowedList,
+} from 'react-window';
+import {
+ OptionTypeBase,
+ OptionProps,
+ MenuListComponentProps,
+} from 'react-select';
+import { ThemeConfig } from '../styles';
+
+export type WindowedMenuListProps = {
+ selectProps: {
+ windowListRef?: RefObject<WindowedList>;
+ optionHeight?: number;
+ };
+};
+
+/**
+ * MenuListComponentProps should always have `children` elements, as guaranteed
+ * by https://github.com/JedWatson/react-select/blob/32ad5c040bdd96cd1ca71010c2558842d684629c/packages/react-select/src/Select.js#L1686-L1719
+ *
+ * `children` may also be `Component<NoticeProps<OptionType>>` if options are not
+ * provided (e.g., when async list is still loading, or no results), but that's
+ * not possible because this MenuList will only be rendered when
+ * optionsLength > windowThreshold.
+ *
+ * If may also be `Component<GroupProps<OptionType>>[]` but we are not supporting
+ * grouped options just yet.
+ */
+export type MenuListProps<
+ OptionType extends OptionTypeBase
+> = MenuListComponentProps<OptionType> & {
+ children: Component<OptionProps<OptionType>>[];
+ // theme is not present with built-in @types/react-select, but is actually
+ // available via CommonProps.
+ theme?: ThemeConfig;
+ className?: string;
+} & WindowedMenuListProps;
+
+/**
+ * Get the index of the last selected option.
+ */
+function getLastSelected(children: Component<any>[]) {
+ return Array.isArray(children)
+ ? Math.max(
+ children.findIndex(
+ ({ props: { isFocused = false } = {} }) => isFocused,
+ ),
+ 0,
+ )
+ : -1;
+}
+
+/**
+ * Calculate probable option height as set in theme configs
+ */
+function detectHeight({ spacing: { baseUnit, lineHeight } }: ThemeConfig) {
+ return baseUnit * 4 + lineHeight;
+}
+
+export default function WindowedMenuList<OptionType extends OptionTypeBase>({
+ children,
+ ...props
+}: MenuListProps<OptionType>) {
+ const {
+ maxHeight,
+ selectProps,
+ theme,
+ getStyles,
+ cx,
+ innerRef,
+ isMulti,
+ className,
+ } = props;
+ const {
+ // Expose react-window VariableSizeList instance and HTML elements
+ windowListRef = useRef(null),
+ windowListInnerRef,
+ } = selectProps;
+
+ // try get default option height from theme configs
+ let optionHeight = selectProps.optionHeight;
+ if (!optionHeight) {
+ optionHeight = theme ? detectHeight(theme) : 30;
Review comment:
let's make 30 a constant here
##########
File path: superset-frontend/cypress-base/cypress/integration/dashboard/filter.js
##########
@@ -53,12 +53,11 @@ export default () =>
});
it('should apply filter', () => {
- cy.wait(10);
-
- cy.get('.Select-placeholder')
+ cy.get('.Select__control')
.contains('Select [region]')
- .click()
- .next()
+ .click({ force: true })
+ // parent of placeholder component
Review comment:
I feel like this comment isn't necessary because `.parent()` is pretty obvious what it does, but up to you
##########
File path: superset-frontend/src/components/Select/SupersetStyledSelect.tsx
##########
@@ -0,0 +1,301 @@
+/**
+ * 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, { SyntheticEvent, MutableRefObject } from 'react';
+import { merge } from 'lodash';
+import BasicSelect, {
+ OptionTypeBase,
+ MultiValueProps,
+ FormatOptionLabelMeta,
+ ValueType,
+ SelectComponentsConfig,
+ components as defaultComponents,
+ createFilter,
+} from 'react-select';
+import Async from 'react-select/async';
+import Creatable from 'react-select/creatable';
+import AsyncCreatable from 'react-select/async-creatable';
+
+import { SelectComponents } from 'react-select/src/components';
+import {
+ SortableContainer,
+ SortableElement,
+ SortableContainerProps,
+} from 'react-sortable-hoc';
+import arrayMove from 'array-move';
+import {
+ WindowedSelectComponentType,
+ WindowedSelectProps,
+ WindowedSelect,
+ WindowedAsyncSelect,
+ WindowedCreatableSelect,
+ WindowedAsyncCreatableSelect,
+} from './WindowedSelect';
+import {
+ DEFAULT_CLASS_NAME,
+ DEFAULT_CLASS_NAME_PREFIX,
+ DEFAULT_STYLES,
+ DEFAULT_THEME,
+ DEFAULT_COMPONENTS,
+ VALUE_LABELED_STYLES,
+ PartialThemeConfig,
+ PartialStylesConfig,
+} from './styles';
+import { findValue } from './utils';
+
+const DEFAULT_WINDOW_THRESHOLD = 100;
+
+type AnyReactSelect<OptionType extends OptionTypeBase> =
+ | BasicSelect<OptionType>
+ | Async<OptionType>
+ | Creatable<OptionType>
+ | AsyncCreatable<OptionType>;
+
+export type SupersetStyledSelectProps<
+ OptionType extends OptionTypeBase,
+ T extends WindowedSelectProps<OptionType> = WindowedSelectProps<OptionType>
+> = T & {
+ // additional props for easier usage or backward compatibility
+ labelKey?: string;
+ valueKey?: string;
+ multi?: boolean;
+ clearable?: boolean;
+ sortable?: boolean;
+ ignoreAccents?: boolean;
+ creatable?: boolean;
+ selectRef?:
+ | React.RefCallback<AnyReactSelect<OptionType>>
+ | MutableRefObject<AnyReactSelect<OptionType>>;
+ getInputValue?: (selectBalue: ValueType<OptionType>) => string | undefined;
+ optionRenderer?: (option: OptionType) => React.ReactNode;
+ valueRenderer?: (option: OptionType) => React.ReactNode;
+ valueRenderedAsLabel?: boolean;
+ // callback for paste event
+ onPaste?: (e: SyntheticEvent) => void;
+ // for simplier theme overrides
+ themeConfig?: PartialThemeConfig;
+ stylesConfig?: PartialStylesConfig;
+};
+
+function styled<
+ OptionType extends OptionTypeBase,
+ SelectComponentType extends WindowedSelectComponentType<
+ OptionType
+ > = WindowedSelectComponentType<OptionType>
+>(SelectComponent: SelectComponentType) {
+ type SelectProps = SupersetStyledSelectProps<OptionType>;
+ type Components = SelectComponents<OptionType>;
+
+ const SortableSelectComponent = SortableContainer(SelectComponent, {
+ withRef: true,
+ });
+
+ // default components for the given OptionType
+ const supersetDefaultComponents: SelectComponentsConfig<OptionType> = DEFAULT_COMPONENTS;
+
+ const getSortableMultiValue = (MultiValue: Components['MultiValue']) => {
+ return SortableElement((props: MultiValueProps<OptionType>) => {
+ const onMouseDown = (e: SyntheticEvent) => {
+ e.preventDefault();
+ e.stopPropagation();
+ };
+ const innerProps = { onMouseDown };
+ return <MultiValue {...props} innerProps={innerProps} />;
+ });
+ };
+
+ /**
+ * Superset styled `Select` component. Apply Superset themed stylesheets and
+ * consolidate props API for backward compatibility with react-select v1.
+ */
+ function StyledSelect(selectProps: SelectProps) {
+ let stateManager: AnyReactSelect<OptionType>; // reference to react-select StateManager
+ const {
+ selectRef,
+ options,
+ value: value_,
+ className = DEFAULT_CLASS_NAME,
+ classNamePrefix = DEFAULT_CLASS_NAME_PREFIX,
+ themeConfig,
+ stylesConfig = {},
+
Review comment:
style nits, but all this white space inside the object destructure is a bit hard to read. I think with the comments we could remove all the white space here and there would still be a good separation of categories. up to you though
##########
File path: superset-frontend/src/components/ListView/Filters.tsx
##########
@@ -37,13 +44,21 @@ interface SelectFilterProps extends BaseFilter {
const FilterContainer = styled.div`
display: inline-flex;
- margin-right: 8px;
+ margin-right: 1.2em;
Review comment:
Are we using `em`s instead of `px`s now? What led to this change?
##########
File path: superset-frontend/src/components/Select/WindowedSelect/WindowedMenuList.tsx
##########
@@ -0,0 +1,155 @@
+/**
+ * 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, {
+ useRef,
+ useEffect,
+ Component,
+ FunctionComponent,
+ RefObject,
+} from 'react';
+import {
+ ListChildComponentProps,
+ FixedSizeList as WindowedList,
+} from 'react-window';
+import {
+ OptionTypeBase,
+ OptionProps,
+ MenuListComponentProps,
+} from 'react-select';
+import { ThemeConfig } from '../styles';
+
+export type WindowedMenuListProps = {
+ selectProps: {
+ windowListRef?: RefObject<WindowedList>;
+ optionHeight?: number;
+ };
+};
+
+/**
+ * MenuListComponentProps should always have `children` elements, as guaranteed
+ * by https://github.com/JedWatson/react-select/blob/32ad5c040bdd96cd1ca71010c2558842d684629c/packages/react-select/src/Select.js#L1686-L1719
+ *
+ * `children` may also be `Component<NoticeProps<OptionType>>` if options are not
+ * provided (e.g., when async list is still loading, or no results), but that's
+ * not possible because this MenuList will only be rendered when
+ * optionsLength > windowThreshold.
+ *
+ * If may also be `Component<GroupProps<OptionType>>[]` but we are not supporting
+ * grouped options just yet.
+ */
+export type MenuListProps<
+ OptionType extends OptionTypeBase
+> = MenuListComponentProps<OptionType> & {
+ children: Component<OptionProps<OptionType>>[];
+ // theme is not present with built-in @types/react-select, but is actually
+ // available via CommonProps.
+ theme?: ThemeConfig;
+ className?: string;
+} & WindowedMenuListProps;
+
+/**
+ * Get the index of the last selected option.
+ */
+function getLastSelected(children: Component<any>[]) {
+ return Array.isArray(children)
+ ? Math.max(
+ children.findIndex(
+ ({ props: { isFocused = false } = {} }) => isFocused,
+ ),
+ 0,
+ )
+ : -1;
+}
+
+/**
+ * Calculate probable option height as set in theme configs
+ */
+function detectHeight({ spacing: { baseUnit, lineHeight } }: ThemeConfig) {
+ return baseUnit * 4 + lineHeight;
Review comment:
what does 4 represent? can we make it a constant?
##########
File path: superset-frontend/src/explore/constants.js
##########
@@ -53,10 +55,10 @@ export const HAVING_OPERATORS = [
OPERATORS['>='],
OPERATORS['<='],
];
-export const MULTI_OPERATORS = [OPERATORS.in, OPERATORS['not in']];
+export const MULTI_OPERATORS = new Set([OPERATORS.in, OPERATORS['not in']]);
Review comment:
not changed here, but I believe @villebro standardized the operators to be `IN` and `NOT IN`. Perhaps that change should be made?
##########
File path: superset-frontend/src/components/Select/styles.tsx
##########
@@ -0,0 +1,299 @@
+/**
+ * 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, { CSSProperties } from 'react';
+import { css, SerializedStyles } from '@emotion/core';
+import { supersetTheme } from '@superset-ui/style';
+import {
+ Styles,
+ Theme,
+ SelectComponentsConfig,
+ components as defaultComponents,
+} from 'react-select';
+import { colors as reactSelectColros } from 'react-select/src/theme';
+import { supersetColors } from 'src/components/styles';
+
+export const DEFAULT_CLASS_NAME = 'Select';
+export const DEFAULT_CLASS_NAME_PREFIX = 'Select';
+
+type RecursivePartial<T> = {
Review comment:
this seems like it could be useful in a global types file. does one exist yet? if not should we make one?
##########
File path: superset-frontend/src/components/Select/styles.tsx
##########
@@ -0,0 +1,299 @@
+/**
+ * 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, { CSSProperties } from 'react';
+import { css, SerializedStyles } from '@emotion/core';
+import { supersetTheme } from '@superset-ui/style';
+import {
+ Styles,
+ Theme,
+ SelectComponentsConfig,
+ components as defaultComponents,
+} from 'react-select';
+import { colors as reactSelectColros } from 'react-select/src/theme';
+import { supersetColors } from 'src/components/styles';
+
+export const DEFAULT_CLASS_NAME = 'Select';
+export const DEFAULT_CLASS_NAME_PREFIX = 'Select';
+
+type RecursivePartial<T> = {
+ [P in keyof T]?: RecursivePartial<T[P]>;
+};
+
+export type ThemeConfig = {
+ borderRadius: number;
+ // z-index for menu dropdown
+ // (the same as `@z-index-above-dashboard-charts + 1` in variables.less)
+ zIndex: number;
+ colors: {
+ // add known colors
+ [key in keyof typeof reactSelectColros]: string;
+ } &
+ {
+ [key in keyof typeof supersetColors]: string;
+ } & {
+ [key: string]: string; // any other colors
+ };
+ spacing: Theme['spacing'] & {
+ // line height and font size must be pixels for easier computation
+ // of option item height in WindowedMenuList
+ lineHeight: number;
+ fontSize: number;
+ // other relative size must be string
+ minWidth: string;
+ };
+};
+
+export type PartialThemeConfig = RecursivePartial<ThemeConfig>;
+
+export const DEFAULT_THEME: PartialThemeConfig = {
+ borderRadius: parseInt(supersetTheme.borderRadius, 10),
+ zIndex: 11,
+ colors: {
+ ...supersetColors,
+ dangerLight: supersetColors.warning,
+ },
+ spacing: {
+ baseUnit: 3,
+ menuGutter: 0,
+ controlHeight: 28,
+ lineHeight: 19,
+ fontSize: 14,
+ minWidth: '7.5em', // just enough to display 'No options'
+ },
+};
+
+// let styles accept serialized CSS, too
+type CSSStyles = CSSProperties | SerializedStyles;
+type styleFnWithSerializedStyles = (
+ base: CSSProperties,
+ state: any,
+) => CSSStyles | CSSStyles[];
+
+export type StylesConfig = {
+ [key in keyof Styles]: styleFnWithSerializedStyles;
+};
+export type PartialStylesConfig = Partial<StylesConfig>;
+
+// Please install "vscode-styled-components" to get properly syntax highlight
Review comment:
let's remove this comment and add it to CONTRIBUTING.md
----------------------------------------------------------------
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