ktmud commented on a change in pull request #13210: URL: https://github.com/apache/superset/pull/13210#discussion_r582207003
########## File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectLabel.tsx ########## @@ -0,0 +1,118 @@ +/** + * 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, { useState } from 'react'; +import { useDrop } from 'react-dnd'; +import { isEmpty } from 'lodash'; +import { t, withTheme, SupersetTheme } from '@superset-ui/core'; +import { BaseControlConfig, ColumnMeta } from '@superset-ui/chart-controls'; +import ControlHeader from 'src/explore/components/ControlHeader'; +import { + AddControlLabel, + HeaderContainer, + LabelsContainer, +} from 'src/explore/components/OptionControls'; +import { + DatasourcePanelDndType, + DatasourcePanelDndItem, +} from 'src/explore/components/DatasourcePanel/types'; +import Icon from 'src/components/Icon'; +import OptionWrapper from './components/OptionWrapper'; +import { OptionSelector } from './utils'; + +interface LabelProps extends BaseControlConfig { + name: string; + value: string[] | string | null; + onChange: (value: string[] | string | null) => void; + options: { string: ColumnMeta }; + theme: SupersetTheme; +} + +function DndColumnSelectLabel(props: LabelProps) { + const { value, options } = props; + const optionSelector = new OptionSelector(options, value); + const [groupByOptions, setGroupByOptions] = useState<ColumnMeta[]>( + optionSelector.groupByOptions, + ); + + const [, datasourcePanelDrop] = useDrop({ + accept: DatasourcePanelDndType.COLUMN, + + drop: (item: DatasourcePanelDndItem) => { + optionSelector.add(item.metricOrColumnName); + setGroupByOptions(optionSelector.groupByOptions); + props.onChange(optionSelector.getValues()); + }, + + canDrop: (item: DatasourcePanelDndItem) => + !optionSelector.has(item.metricOrColumnName), + + collect: monitor => ({ + isOver: monitor.isOver(), + canDrop: monitor.canDrop(), + }), + }); + + function onClickClose(index: number) { + optionSelector.del(index); + setGroupByOptions(optionSelector.groupByOptions); + props.onChange(optionSelector.getValues()); + } + + function onShiftOptions(dragIndex: number, hoverIndex: number) { + optionSelector.swap(dragIndex, hoverIndex); + setGroupByOptions(optionSelector.groupByOptions); + props.onChange(optionSelector.getValues()); + } + + function placeHolderRenderer() { + return ( + <AddControlLabel cancelHover> Review comment: @junlincc Trust me, most users are lazy and would not want to type. Browsing the list to add columns is probably a much more common use case than you thought. For example, in Airbnb, in one of the datasources, there are many metrics with "nights_booked" in the name, when users want to find a specific metric related to "nights_booked" but couldn't remember the exact name, even after they typed "nights_booked", they may still need to browse through more than 50 options. Re: offering DnD only or both interactions: I think one of the biggest advantage of Superset is the ability to create adhoc metric very easily. I'm struggling to imagine how that workflow of conveniently creating adhoc metrics/columns would look like without a popover. If we are going to build a popover anyway, why not keep both DnD and click? With proper design and abstraction, the code complexity should be manageable. Re: code maintenance: That said, current code indeed is very complex, and is not even in TypeScript, so I'm in support of writing the new control bit by bit from scratch. But I also think it would make sense to be generic from the get go and make sure the architecture can support all use cases---because we already have so much learning from the existing controls. It would be nice if we can focus on collecting all feature requests first and do an architecture review (maybe in a SIP?) before diving deep into implementing the full feature for a specific control type. This POC is a good starting point to kick off the conversation, but let's make sure we get things right to avoid wasted work. I'm also not sure how to run user testing in this case, do we ask users to choose between DnD and click? Do we do that after all features for DnD are implemented or before? What if users pick click because DnD is not easy to use enough? These controls need an overhaul anyway so I can't imagine a case where we will abandon the new solution just because of some unfavorable use feedbacks (which are probably fixable). It is also not true that we will need to spend 2x of time to maintain two sets of features. Current select controls can have a code freeze while we work on the new controls with more advanced features and better architecture. We would probably need that to stay focused. ---------------------------------------------------------------- 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]
