rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3430424562
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx:
##########
@@ -2345,299 +2300,289 @@ class DatasourceEditor extends PureComponent<
/>
</div>
);
- }
+ }, [datasource, sortMetrics, onDatasourcePropChange, metricSearchTerm]);
- render() {
- const { datasource, activeTabKey } = this.state;
- const { metrics } = datasource;
- const sortedMetrics = metrics?.length ? this.sortMetrics(metrics) : [];
+ const sortedMetrics = useMemo(
+ () => (datasource.metrics?.length ? sortMetrics(datasource.metrics) : []),
+ [datasource.metrics, sortMetrics],
+ );
- return (
- <DatasourceContainer data-test="datasource-editor">
- {this.renderErrors()}
- <Alert
- css={theme => ({ marginBottom: theme.sizeUnit * 4 })}
- type="warning"
- message={
- <>
- {' '}
- <strong>{t('Be careful.')} </strong>
- {t(
- 'Changing these settings will affect all charts using this
dataset, including charts owned by other people.',
- )}
- </>
- }
+ // Retained to mirror the canonical (class-based) component on master: the
+ // Spatial tab definition is kept available even though it is not wired into
+ // the rendered tab list. Removing it would also drop its translatable
strings
+ // and regress existing translations. It is referenced in the `tabItems`
+ // dependency list below so it is not reported as an unused local.
+ const renderSpatialTab = useCallback(() => {
+ const { spatials, all_cols: allCols } = datasource;
+
+ return {
+ key: TABS_KEYS.SPATIAL,
+ label: <CollectionTabTitle collection={spatials} title={t('Spatial')} />,
+ children: (
+ <CollectionTable
+ tableColumns={['name', 'config']}
+ sortColumns={['name']}
+ onChange={value => onDatasourcePropChange('spatials', value)}
+ itemGenerator={() => ({
+ name: t('<new spatial>'),
+ type: t('<no type>'),
+ config: null,
+ })}
+ collection={spatials ?? []}
+ allowDeletes
+ itemRenderers={{
+ name: (d, onChange) => (
+ <EditableTitle
+ canEdit
+ title={d as string}
+ onSaveTitle={onChange}
+ />
+ ),
+ config: (v, onChange) => (
+ <SpatialControl
+ value={
+ v as {
+ type: 'latlong' | 'delimited' | 'geohash';
+ }
+ }
+ onChange={onChange}
+ choices={allCols?.map(col => [col, col] as [string, string])}
+ />
+ ),
+ }}
/>
- <StyledTableTabs
- id="table-tabs"
- data-test="edit-dataset-tabs"
- onChange={this.handleTabSelect}
- defaultActiveKey={activeTabKey}
- items={[
- {
- key: TABS_KEYS.SOURCE,
- label: t('Source'),
- children: this.renderSourceFieldset(),
- },
- {
- key: TABS_KEYS.METRICS,
- label: (
- <CollectionTabTitle
- collection={sortedMetrics}
- title={t('Metrics')}
- />
- ),
- children: this.renderMetricCollection(),
- },
- {
- key: TABS_KEYS.COLUMNS,
- label: (
- <CollectionTabTitle
- collection={this.state.databaseColumns}
- title={t('Columns')}
- />
- ),
- children: (
- <StyledTableTabWrapper>
- {this.renderDefaultColumnSettings()}
- <ColumnButtonWrapper>
- <StyledButtonWrapper>
- <Button
- buttonSize="small"
- buttonStyle="tertiary"
- onClick={this.syncMetadata}
- className="sync-from-source"
- disabled={this.state.isEditMode}
- >
- <Icons.DatabaseOutlined iconSize="m" />
- {t('Sync columns from source')}
- </Button>
- </StyledButtonWrapper>
- </ColumnButtonWrapper>
- <Input.Search
- placeholder={t('Search columns by name')}
- value={this.state.columnSearchTerm}
- onChange={e =>
- this.setState({ columnSearchTerm: e.target.value })
- }
- style={{ marginBottom: 16, width: 300 }}
- allowClear
- />
- <ColumnCollectionTable
- className="columns-table"
- columns={this.state.databaseColumns}
- filterTerm={this.state.columnSearchTerm}
- filterFields={['column_name']}
- datasource={datasource}
- onColumnsChange={databaseColumns =>
- this.setColumns({ databaseColumns })
- }
- onDatasourceChange={this.onDatasourceChange}
- />
- {this.state.metadataLoading && <Loading />}
- </StyledTableTabWrapper>
- ),
- },
+ ),
+ };
+ }, [datasource, onDatasourcePropChange]);
+
+ const tabItems = useMemo(
+ () => [
+ {
+ key: TABS_KEYS.SOURCE,
+ label: t('Source'),
+ children: renderSourceFieldset(),
+ },
+ {
+ key: TABS_KEYS.METRICS,
+ label: (
+ <CollectionTabTitle collection={sortedMetrics} title={t('Metrics')}
/>
+ ),
+ children: renderMetricCollection(),
+ },
+ {
+ key: TABS_KEYS.COLUMNS,
+ label: (
+ <CollectionTabTitle
+ collection={databaseColumns}
+ title={t('Columns')}
+ />
+ ),
+ children: (
+ <StyledTableTabWrapper>
+ {renderDefaultColumnSettings()}
+ <DefaultColumnSettingsTitle>
+ {t('Column Settings')}
+ </DefaultColumnSettingsTitle>
+ <ColumnButtonWrapper>
+ <StyledButtonWrapper>
+ <Button
+ buttonSize="small"
+ buttonStyle="tertiary"
+ onClick={syncMetadata}
+ className="sync-from-source"
+ disabled={isEditMode}
Review Comment:
`disabled={isEditMode}` is exactly what master's class component has
(`disabled={this.state.isEditMode}`) — the conversion preserved it faithfully.
Whether the condition is actually inverted is a separate question from the FC
move, so I'd rather not flip behavior here.
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -52,18 +52,18 @@ const StyledButtonWrapper = styled.span`
`}
`;
-type CollectionItem = { id: string | number; [key: string]: any };
+type CollectionItem = { id: string | number; [key: string]: unknown };
function createKeyedCollection(arr: Array<object>) {
const collectionArray = arr.map(
- (o: any) =>
+ (o: Record<string, unknown>) =>
({
...o,
id: o.id || nanoid(),
Review Comment:
`id: o.id || nanoid()` is identical to master's class version — collection
ids in this codebase aren't `0`, so the falsy-zero case isn't reachable in
practice. Preserving the existing behavior for the conversion.
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -74,270 +74,310 @@ function createKeyedCollection(arr: Array<object>) {
};
}
-export default class CRUDCollection extends PureComponent<
- CRUDCollectionProps,
- CRUDCollectionState
-> {
- constructor(props: CRUDCollectionProps) {
- super(props);
-
- const { collection, collectionArray } = createKeyedCollection(
- props.collection,
- );
-
- // Get initial page size from pagination prop
- const initialPageSize =
- typeof props.pagination === 'object' && props.pagination?.pageSize
- ? props.pagination.pageSize
- : 10;
-
- this.state = {
- expandedColumns: {},
- collection,
- collectionArray,
- sortColumn: '',
- sort: 0,
- currentPage: 1,
- pageSize: initialPageSize,
- };
- this.onAddItem = this.onAddItem.bind(this);
- this.renderExpandableSection = this.renderExpandableSection.bind(this);
- this.getLabel = this.getLabel.bind(this);
- this.onFieldsetChange = this.onFieldsetChange.bind(this);
- this.changeCollection = this.changeCollection.bind(this);
- this.handleTableChange = this.handleTableChange.bind(this);
- this.buildTableColumns = this.buildTableColumns.bind(this);
- this.toggleExpand = this.toggleExpand.bind(this);
- }
-
- componentDidUpdate(prevProps: CRUDCollectionProps) {
- if (this.props.collection !== prevProps.collection) {
- const { collection, collectionArray } = createKeyedCollection(
- this.props.collection,
- );
+export default function CRUDCollection({
+ allowAddItem = false,
+ allowDeletes = false,
+ collection: propsCollection,
+ columnLabels,
+ columnLabelTooltips,
+ emptyMessage = t('No items'),
+ expandFieldset,
+ itemGenerator,
+ itemCellProps,
+ itemRenderers,
+ onChange,
+ tableColumns,
+ sortColumns = [],
+ stickyHeader = false,
+ pagination = false,
+ filterTerm,
+ filterFields,
+}: CRUDCollectionProps) {
+ const [expandedColumns, setExpandedColumns] = useState<
+ Record<PropertyKey, boolean>
+ >({});
+ const [collection, setCollection] = useState<
+ Record<PropertyKey, CollectionItem>
+ >(() => createKeyedCollection(propsCollection).collection);
+ const [collectionArray, setCollectionArray] = useState<CollectionItem[]>(
+ () => createKeyedCollection(propsCollection).collectionArray,
+ );
+ const [sortColumn, setSortColumn] = useState<string>('');
+ const [sort, setSort] = useState<SortOrderEnum>(SortOrderEnum.Unsorted);
+ // Controlled pagination: tracked so that filtering can clamp currentPage
+ // back to a valid page (avoids the user being stranded on an empty page
+ // when filterTerm shrinks the result set).
+ const [pageSize, setPageSize] = useState<number>(() =>
+ typeof pagination === 'object' && pagination?.pageSize
+ ? pagination.pageSize
+ : 10,
+ );
+ const [currentPage, setCurrentPage] = useState<number>(1);
+
+ // Sync with props.collection changes
+ useEffect(() => {
+ const { collection: newCollection, collectionArray: newCollectionArray } =
+ createKeyedCollection(propsCollection);
+ setCollection(newCollection);
+ setCollectionArray(newCollectionArray);
+ }, [propsCollection]);
+
+ const onCellChange = useCallback(
+ (id: string | number, col: string, val: unknown) => {
+ setCollection(prevCollection => {
+ const updatedCollection = {
+ ...prevCollection,
+ [id]: {
+ ...prevCollection[id],
+ [col]: val,
+ },
+ };
+ return updatedCollection;
+ });
- this.setState(prevState => ({
- collection,
- collectionArray,
- expandedColumns: prevState.expandedColumns,
- }));
- }
- }
-
- onCellChange(id: string | number, col: string, val: unknown) {
- this.setState(prevState => {
- const updatedCollection = {
- ...prevState.collection,
- [id]: {
- ...prevState.collection[id],
- [col]: val,
- },
- };
- const updatedCollectionArray = prevState.collectionArray.map(item =>
- item.id === id ? updatedCollection[id] : item,
- );
+ setCollectionArray(prevCollectionArray => {
+ const updatedCollectionArray = prevCollectionArray.map(item => {
+ if (item.id === id) {
+ return {
+ ...item,
+ [col]: val,
+ };
+ }
+ return item;
+ });
+
+ if (onChange) {
+ onChange(updatedCollectionArray);
+ }
+
+ return updatedCollectionArray;
+ });
+ },
+ [onChange],
+ );
- if (this.props.onChange) {
- this.props.onChange(updatedCollectionArray);
+ const changeCollection = useCallback(
+ (
+ newCollection: Record<PropertyKey, CollectionItem>,
+ currentCollectionArray: CollectionItem[],
+ ) => {
+ // Preserve existing order instead of recreating from Object.keys()
+ const existingIds = new Set(currentCollectionArray.map(item => item.id));
+ const newCollectionArray: CollectionItem[] = [];
+
+ // First pass: preserve existing order and update items
+ for (const existingItem of currentCollectionArray) {
+ if (newCollection[existingItem.id]) {
+ newCollectionArray.push(newCollection[existingItem.id]);
+ }
}
- return {
- collection: updatedCollection,
- collectionArray: updatedCollectionArray,
- };
- });
- }
- onAddItem() {
- if (this.props.itemGenerator) {
- let newItem = this.props.itemGenerator();
+ // Second pass: add new items
+ for (const item of Object.values(newCollection)) {
+ if (!existingIds.has(item.id)) {
+ newCollectionArray.push(item);
+ }
+ }
+
+ setCollection(newCollection);
+ setCollectionArray(newCollectionArray);
+
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
+ },
+ [onChange],
+ );
+
+ const deleteItem = useCallback(
+ (id: string | number) => {
+ setCollection(prevCollection => {
+ const newColl = { ...prevCollection };
+ delete newColl[id];
+ return newColl;
+ });
+
+ setCollectionArray(prevCollectionArray => {
+ const newCollectionArray = prevCollectionArray.filter(
+ item => item.id !== id,
+ );
+
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
+
+ return newCollectionArray;
+ });
+ },
+ [onChange],
+ );
+
+ const onAddItem = useCallback(() => {
+ if (itemGenerator) {
+ let newItem = itemGenerator() as CollectionItem;
const shouldStartExpanded = newItem.expanded === true;
if (!newItem.id) {
newItem = { ...newItem, id: nanoid() };
}
delete newItem.expanded;
- this.setState(
- prevState => {
- const newCollection = {
- ...prevState.collection,
- [newItem.id]: newItem,
- };
- const newExpandedColumns = shouldStartExpanded
- ? { ...prevState.expandedColumns, [newItem.id]: true }
- : prevState.expandedColumns;
- const newCollectionArray = [newItem, ...prevState.collectionArray];
-
- return {
- collection: newCollection,
- collectionArray: newCollectionArray,
- expandedColumns: newExpandedColumns,
- };
- },
- () => {
- if (this.props.onChange) {
- this.props.onChange(this.state.collectionArray);
- }
- },
- );
- }
- }
+ setCollection(prevCollection => ({
+ ...prevCollection,
+ [newItem.id]: newItem,
+ }));
- onFieldsetChange(item: any) {
- this.changeCollection({
- ...this.state.collection,
- [item.id]: item,
- });
- }
+ setCollectionArray(prevCollectionArray => {
+ const newCollectionArray = [newItem, ...prevCollectionArray];
- getLabel(col: any): string {
- const { columnLabels } = this.props;
- let label = columnLabels?.[col] ? columnLabels[col] : col;
- if (label.startsWith('__')) {
- label = '';
- }
- return label;
- }
-
- getTooltip(col: string): string | undefined {
- const { columnLabelTooltips } = this.props;
- return columnLabelTooltips?.[col];
- }
-
- changeCollection(collection: any) {
- // Preserve existing order instead of recreating from Object.keys()
- const existingIds = new Set(
- this.state.collectionArray.map(item => item.id),
- );
- const newCollectionArray: CollectionItem[] = [];
-
- // First pass: preserve existing order and update items
- for (const existingItem of this.state.collectionArray) {
- if (collection[existingItem.id]) {
- newCollectionArray.push(collection[existingItem.id]);
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
+
+ return newCollectionArray;
+ });
+
+ if (shouldStartExpanded) {
+ setExpandedColumns(prev => ({ ...prev, [newItem.id]: true }));
}
}
+ }, [itemGenerator, onChange]);
+
+ const onFieldsetChange = useCallback(
+ (item: CollectionItem) => {
+ changeCollection(
+ {
+ ...collection,
+ [item.id]: item,
+ },
+ collectionArray,
+ );
+ },
+ [changeCollection, collection, collectionArray],
+ );
- // Second pass: add new items
- for (const item of Object.values(collection) as CollectionItem[]) {
- if (!existingIds.has(item.id)) {
- newCollectionArray.push(item);
+ const getLabel = useCallback(
+ (col: string): string => {
+ let label = columnLabels?.[col] ? columnLabels[col] : col;
+ if (label.startsWith('__')) {
+ label = '';
}
- }
+ return label;
+ },
+ [columnLabels],
+ );
- this.setState({ collection, collectionArray: newCollectionArray });
+ const getTooltip = useCallback(
+ (col: string): string | undefined => columnLabelTooltips?.[col],
+ [columnLabelTooltips],
+ );
- if (this.props.onChange) {
- this.props.onChange(newCollectionArray);
- }
- }
-
- deleteItem(id: string | number) {
- const newColl = { ...this.state.collection };
- delete newColl[id];
- this.changeCollection(newColl);
- }
-
- toggleExpand(id: any) {
- this.setState(prevState => ({
- expandedColumns: {
- ...prevState.expandedColumns,
- [id]: !prevState.expandedColumns[id],
- },
+ const toggleExpand = useCallback((id: string | number) => {
+ setExpandedColumns(prev => ({
+ ...prev,
+ [id]: !prev[id],
}));
- }
-
- handleTableChange(
- pagination: TablePaginationConfig,
- _filters: Record<string, FilterValue | null>,
- sorter: SorterResult<CollectionItem> | SorterResult<CollectionItem>[],
- ) {
- // Handle pagination changes
- if (pagination.current !== undefined && pagination.pageSize !== undefined)
{
- this.setState({
- currentPage: pagination.current,
- pageSize: pagination.pageSize,
- });
- }
-
- // Handle sorting changes
- const columnSorter = Array.isArray(sorter) ? sorter[0] : sorter;
- let newSortColumn = '';
- let newSortOrder = 0;
-
- if (columnSorter?.columnKey && columnSorter?.order) {
- newSortColumn = columnSorter.columnKey as string;
- newSortOrder = columnSorter.order === 'ascend' ? 1 : 2;
- }
-
- const { sortColumns } = this.props;
- const col = newSortColumn;
+ }, []);
+
+ const handleTableChange = useCallback(
+ (
+ paginationEvt: TablePaginationConfig,
+ _filters: Record<string, FilterValue | null>,
+ sorter: SorterResult<CollectionItem> | SorterResult<CollectionItem>[],
+ ) => {
+ if (
+ paginationEvt.current !== undefined &&
+ paginationEvt.pageSize !== undefined
+ ) {
+ setCurrentPage(paginationEvt.current);
+ setPageSize(paginationEvt.pageSize);
+ }
+ const columnSorter = Array.isArray(sorter) ? sorter[0] : sorter;
+ let newSortColumn = '';
+ let newSortOrder = SortOrderEnum.Unsorted;
+
+ if (columnSorter?.columnKey && columnSorter?.order) {
+ newSortColumn = columnSorter.columnKey as string;
+ newSortOrder =
+ columnSorter.order === 'ascend'
+ ? SortOrderEnum.Asc
+ : SortOrderEnum.Desc;
+ }
- if (sortColumns?.includes(col) || newSortOrder === 0) {
- let sortedArray = [...this.props.collection];
+ const col = newSortColumn;
+
+ if (
+ sortColumns?.includes(col) ||
+ newSortOrder === SortOrderEnum.Unsorted
+ ) {
+ let sortedArray = [...propsCollection] as CollectionItem[];
Review Comment:
Sorting from `propsCollection` mirrors master's class component, which
sorted from `this.props.collection` — so this is preserved behavior, not new.
Changing the sort source is a behavior tweak I'd keep out of the conversion.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
