rusackas commented on code in PR #39461:
URL: https://github.com/apache/superset/pull/39461#discussion_r3478101827
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -74,270 +81,317 @@ 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);
+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>
+ >({});
+ // Seed both pieces of state from a single createKeyedCollection() pass so
+ // that items lacking an `id` get one consistent set of synthetic ids
+ // (matching the prior class component, which keyed the collection once).
+ const initialKeyed = useRef<ReturnType<typeof createKeyedCollection>>();
+ if (!initialKeyed.current) {
+ initialKeyed.current = createKeyedCollection(propsCollection);
}
+ const [collection, setCollection] = useState<
+ Record<PropertyKey, CollectionItem>
+ >(() => initialKeyed.current!.collection);
+ const [collectionArray, setCollectionArray] = useState<CollectionItem[]>(
+ () => initialKeyed.current!.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;
+ });
- componentDidUpdate(prevProps: CRUDCollectionProps) {
- if (this.props.collection !== prevProps.collection) {
- const { collection, collectionArray } = createKeyedCollection(
- this.props.collection,
- );
+ setCollectionArray(prevCollectionArray => {
+ const updatedCollectionArray = prevCollectionArray.map(item => {
+ if (item.id === id) {
+ return {
+ ...item,
+ [col]: val,
+ };
+ }
+ return item;
+ });
- this.setState(prevState => ({
- collection,
- collectionArray,
- expandedColumns: prevState.expandedColumns,
- }));
- }
- }
+ if (onChange) {
+ onChange(updatedCollectionArray);
+ }
- 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,
- );
+ 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();
- const shouldStartExpanded = newItem.expanded === true;
- if (!newItem.id) {
- newItem = { ...newItem, id: nanoid() };
+ // Second pass: add new items
+ for (const item of Object.values(newCollection)) {
+ if (!existingIds.has(item.id)) {
+ newCollectionArray.push(item);
+ }
}
- 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(newCollection);
+ setCollectionArray(newCollectionArray);
- onFieldsetChange(item: any) {
- this.changeCollection({
- ...this.state.collection,
- [item.id]: item,
- });
- }
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
+ },
+ [onChange],
+ );
- getLabel(col: any): string {
- const { columnLabels } = this.props;
- let label = columnLabels?.[col] ? columnLabels[col] : col;
- if (label.startsWith('__')) {
- label = '';
- }
- return label;
- }
+ const deleteItem = useCallback(
+ (id: string | number) => {
+ setCollection(prevCollection => {
+ const newColl = { ...prevCollection };
+ delete newColl[id];
+ return newColl;
+ });
- getTooltip(col: string): string | undefined {
- const { columnLabelTooltips } = this.props;
- return columnLabelTooltips?.[col];
- }
+ setCollectionArray(prevCollectionArray => {
+ const newCollectionArray = prevCollectionArray.filter(
+ item => item.id !== id,
+ );
- 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);
+ }
- // Second pass: add new items
- for (const item of Object.values(collection) as CollectionItem[]) {
- if (!existingIds.has(item.id)) {
- newCollectionArray.push(item);
- }
- }
+ return newCollectionArray;
+ });
+ },
+ [onChange],
+ );
- this.setState({ collection, collectionArray: newCollectionArray });
+ 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;
- if (this.props.onChange) {
- this.props.onChange(newCollectionArray);
- }
- }
+ setCollection(prevCollection => ({
+ ...prevCollection,
+ [newItem.id]: newItem,
+ }));
- deleteItem(id: string | number) {
- const newColl = { ...this.state.collection };
- delete newColl[id];
- this.changeCollection(newColl);
- }
+ setCollectionArray(prevCollectionArray => {
+ const newCollectionArray = [newItem, ...prevCollectionArray];
- toggleExpand(id: any) {
- this.setState(prevState => ({
- expandedColumns: {
- ...prevState.expandedColumns,
- [id]: !prevState.expandedColumns[id],
- },
- }));
- }
+ if (onChange) {
+ onChange(newCollectionArray);
+ }
- 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,
+ 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],
+ );
- // Handle sorting changes
- const columnSorter = Array.isArray(sorter) ? sorter[0] : sorter;
- let newSortColumn = '';
- let newSortOrder = 0;
+ const getLabel = useCallback(
+ (col: string): string => {
+ let label = columnLabels?.[col] ? columnLabels[col] : col;
+ if (label.startsWith('__')) {
+ label = '';
+ }
+ return label;
+ },
+ [columnLabels],
+ );
- if (columnSorter?.columnKey && columnSorter?.order) {
- newSortColumn = columnSorter.columnKey as string;
- newSortOrder = columnSorter.order === 'ascend' ? 1 : 2;
- }
+ const getTooltip = useCallback(
+ (col: string): string | undefined => columnLabelTooltips?.[col],
+ [columnLabelTooltips],
+ );
- const { sortColumns } = this.props;
- const col = newSortColumn;
+ const toggleExpand = useCallback((id: string | number) => {
+ setExpandedColumns(prev => ({
+ ...prev,
+ [id]: !prev[id],
+ }));
+ }, []);
+
+ 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:
This one mirrors master - the class version also sorts from
`this.props.collection` (and resets from it), so the gap predates the
conversion rather than being introduced here. Sorting from `collectionArray`
seems more correct to me, but it is a behavior change beyond the mechanical
port. Fold it in here, or split it into its own fix so it gets its own test?
--
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]
