Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(explore): Enable selecting an option not included in suggestions #13029

Merged
merged 3 commits into from
Feb 12, 2021
Merged

fix(explore): Enable selecting an option not included in suggestions #13029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87c6fef
Fix warning tooltip in filters
kgabryje Feb 9, 2021
02db645
Enable selecting an option not included in suggestions
kgabryje Feb 9, 2021
1a64a69
Replace bootstrap class with styled component and theme variable
kgabryje Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions superset-frontend/src/explore/components/OptionControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,11 @@
*/
import React, { useRef } from 'react';
import { useDrag, useDrop, DropTargetMonitor } from 'react-dnd';
import { styled, useTheme } from '@superset-ui/core';
import { ColumnOption } from '@superset-ui/chart-controls';
import { styled, t, useTheme } from '@superset-ui/core';
import {
ColumnOption,
InfoTooltipWithTrigger,
} from '@superset-ui/chart-controls';
import { Tooltip } from 'src/common/components/Tooltip';
import Icon from 'src/components/Icon';
import { savedMetricType } from 'src/explore/components/controls/MetricControl/types';
Expand Down Expand Up @@ -73,6 +76,10 @@ const CloseContainer = styled.div`
cursor: pointer;
`;

const StyledInfoTooltipWithTrigger = styled(InfoTooltipWithTrigger)`
margin: 0 ${({ theme }) => theme.gridUnit}px;
`;

export const HeaderContainer = styled.div`
display: flex;
align-items: center;
Expand Down Expand Up @@ -138,6 +145,7 @@ export const OptionControlLabel = ({
isFunction,
type,
index,
isExtra,
...props
}: {
label: string | React.ReactNode;
Expand All @@ -150,6 +158,7 @@ export const OptionControlLabel = ({
isDraggable?: boolean;
type: string;
index: number;
isExtra?: boolean;
}) => {
const theme = useTheme();
const ref = useRef<HTMLDivElement>(null);
Expand Down Expand Up @@ -236,6 +245,17 @@ export const OptionControlLabel = ({
{isFunction && <Icon name="function" viewBox="0 0 16 11" />}
{getLabelContent()}
</Label>
{isExtra && (
<StyledInfoTooltipWithTrigger
icon="exclamation-triangle"
placement="top"
bsStyle="warning"
tooltip={t(`
This filter was inherited from the dashboard's context.
It won't be saved when saving the chart.
`)}
/>
)}
{isAdhoc && (
<CaretContainer>
<Icon name="caret-right" color={theme.colors.grayscale.light1} />
Expand Down
20 changes: 20 additions & 0 deletions .../src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,12 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
this.refreshComparatorSuggestions = this.refreshComparatorSuggestions.bind(
this,
);
this.clearSuggestionSearch = this.clearSuggestionSearch.bind(this);

this.state = {
suggestions: [],
abortActiveRequest: null,
currentSuggestionSearch: '',
};

this.selectProps = {
Expand Down Expand Up @@ -272,8 +274,13 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
return <FilterDefinitionOption option={option} />;
}

clearSuggestionSearch() {
this.setState({ currentSuggestionSearch: '' });
}

render() {
const { adhocFilter, options, datasource } = this.props;
const { currentSuggestionSearch } = this.state;
let columns = options;
const { subject, operator, comparator } = adhocFilter;
const subjectSelectProps = {
Expand Down Expand Up @@ -379,12 +386,25 @@ export default class AdhocFilterEditPopoverSimpleTabContent extends React.Compon
name="filter-value"
{...comparatorSelectProps}
getPopupContainer={triggerNode => triggerNode.parentNode}
onSearch={val => this.setState({ currentSuggestionSearch: val })}
onSelect={this.clearSuggestionSearch}
onBlur={this.clearSuggestionSearch}
>
{this.state.suggestions.map(suggestion => (
<Select.Option value={suggestion} key={suggestion}>
{suggestion}
</Select.Option>
))}

{/* enable selecting an option not included in suggestions */}
{currentSuggestionSearch &&
!this.state.suggestions.some(
suggestion => suggestion === currentSuggestionSearch,
) && (
<Select.Option value={currentSuggestionSearch}>
{currentSuggestionSearch}
</Select.Option>
)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably don't need this since AntD select has a "tags" mode, which behave similar to a freeForm react-select (CreatableSelect).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that tags allows to select multiple values, and operators like equals should work with a single value. That's why this "hack" was needed

Copy link
Member

@ktmud ktmud Feb 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the clarification! I totally missed that.

Things like this makes me wonder again whether we should revert back to react-select... It seems AntD select simply doesn't have the feature completeness and flexibility we want. If this is about aesthetic improvement, we can always restyle react-select to look like AntD (or whatever new style we like).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your concerns and they are all valid. Since we have gone so far in the migration already, it will be a major time waste reverting back. @kgabryje @villebro @zhaoyongjie let's allocate sometime to investigate and understand limitations of AntD, and have a long-term plan to address those concerns before diving deeper into renovating control panel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, the only places we have replaced react-select are in the AdhocFilter and MetricsControl popovers. There are many more places that are currently using react-select. Please also consider future migration cost if we decide to all-in AntD select.

Note that react-select is still a very much actively maintained library, and have some powerful backers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, from an end user perspective this UX is indistinguishable from something where this would be natively supported without the manually added Select.Option. As this solution is quite simple and not really a hack IMO (we are still using fully standard components here), I feel this is fine as-is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do note though you'd have to do this for everywhere the current CreatableSelect is used, ultimately. So this hack might need to be generalized to a wrapper component if we are to keep the same UX.

It just doesn't feel right to me to introduce a hacky solution when there is a current solution that have worked perfectly fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying we should definitely revert. Just laying out some concerns here.

Copy link
Member

@villebro villebro Feb 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding a wrapper that hides the specifics of how the component actually implements the creatable option sounds like a good option. To my understanding this is essentially how the migration process has been carried out this far, i.e. importing components from src/common/components/, where we start out by just exporting the vanilla component, and later extending the component if needed. This way we can later much easier swap out the implementation of said component if a new better library comes along (or if the current implementation ends up being more problematic than we had envisioned). Granted, the abstracted component's props will end up looking fairly identical with their original implementation, but as we probably end up using only a small subset of the features, migrating to a new implementation will at least be slightly easier than not having any centralized abstraction.

Btw, I looked at what the current react-select implementation looks like in the Filter Box, and that flow seems quite hacky to the end user (see below). The select control that's used in the control panel isn't as bad, but it does go to show that the react-select experience in Superset has not been optimal from a UX perspective, either.
fbox

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @villebro, for further explanation. i honestly don't have enough knowledge to tell which library has less drawbacks or better extensibility. I am sure we can always tweak UI/UX and make improvement on either one. since we landed on AntD, let's stick with it and invest in one direction. also, thanks @ktmud for raising your concerns. Let's leave more time for discussion next time when it comes major changes like this. I read through the SIP and felt that we should have done more research back then......

</SelectWithLabel>
) : (
<Input
Expand Down
1 change: 1 addition & 0 deletions superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterOption.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ const AdhocFilterOption = ({
index={index}
type={OPTION_TYPES.filter}
isAdhoc
isExtra={adhocFilter.isExtra}
/>
</AdhocFilterPopoverTrigger>
);
Expand Down
38 changes: 11 additions & 27 deletions ...rset-frontend/src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@
* under the License.
*/
import React from 'react';
import { t } from '@superset-ui/core';
import { InfoTooltipWithTrigger } from '@superset-ui/chart-controls';

import Popover from 'src/common/components/Popover';
import columnType from 'src/explore/propTypes/columnType';
import adhocMetricType from 'src/explore/components/controls/MetricControl/adhocMetricType';
Expand Down Expand Up @@ -85,30 +82,17 @@ class AdhocFilterPopoverTrigger extends React.PureComponent<
);

return (
<>
{adhocFilter.isExtra && (
<InfoTooltipWithTrigger
icon="exclamation-triangle"
placement="top"
className="m-r-5 text-muted"
tooltip={t(`
This filter was inherited from the dashboard's context.
It won't be saved when saving the chart.
`)}
/>
)}
<Popover
placement="right"
trigger="click"
content={overlayContent}
defaultVisible={this.state.popoverVisible}
visible={this.state.popoverVisible}
onVisibleChange={this.togglePopover}
destroyTooltipOnHide={this.props.createNew}
>
{this.props.children}
</Popover>
</>
<Popover
placement="right"
trigger="click"
content={overlayContent}
defaultVisible={this.state.popoverVisible}
visible={this.state.popoverVisible}
onVisibleChange={this.togglePopover}
destroyTooltipOnHide={this.props.createNew}
>
{this.props.children}
</Popover>
);
}
}
Expand Down