michael-s-molina commented on code in PR #21520: URL: https://github.com/apache/superset/pull/21520#discussion_r1016655936
########## superset-frontend/src/components/Table/sorters.test.ts: ########## @@ -0,0 +1,103 @@ +/** + * 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 { alphabeticalSort, numericalSort } from './sorters'; + +const rows = [ + { + name: 'Deathstar Lamp', + category: 'Lamp', + cost: 75.99, + }, + { + name: 'Desk Lamp', + category: 'Lamp', + cost: 15.99, + }, + { + name: 'Bedside Lamp', + category: 'Lamp', + cost: 15.99, + }, + { name: 'Drafting Desk', category: 'Desk', cost: 125 }, + { name: 'Sit / Stand Desk', category: 'Desk', cost: 275.99 }, +]; + +/** + * NOTE: Sorters for antd table use < 0, 0, > 0 for sorting + * -1 or less means the first item comes after the second item + * 0 means the items sort values is equivalent + * 1 or greater means the first item comes before the second item + */ +test('alphabeticalSort sorts correctly', () => { + expect(alphabeticalSort('name', rows[0], rows[1])).toBe(-1); + expect(alphabeticalSort('name', rows[1], rows[0])).toBe(1); + expect(alphabeticalSort('category', rows[1], rows[0])).toBe(0); +}); + +test('numericalSort sorts correctly', () => { + expect(numericalSort('cost', rows[1], rows[2])).toBe(0); + expect(numericalSort('cost', rows[1], rows[0])).toBeLessThan(0); + expect(numericalSort('cost', rows[4], rows[1])).toBeGreaterThan(0); +}); + +/** + * We want to make sure our sorters do not throw runtime errors given bad inputs. + * Runtime Errors in a sorter will cause a catastrophic React lifecycle error and produce white screen of death + * In the case the sorter cannot perform the comparison it should return undefined and the next sort step will proceed without error + */ +test('alphabeticalSort bad inputs no errors', () => { + // @ts-ignore + expect(alphabeticalSort('name', null, null)).toBe(undefined); + // incorrect non-object values + // @ts-ignore + expect(alphabeticalSort('name', 3, [])).toBe(undefined); + // incorrect object values without specificed key + // @ts-ignore Review Comment: ```suggestion ``` ########## superset-frontend/src/components/Table/Table.test.tsx: ########## @@ -0,0 +1,81 @@ +/** + * 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 from 'react'; +import { render, screen } from 'spec/helpers/testing-library'; Review Comment: ```suggestion import { render, screen, waitFor } from 'spec/helpers/testing-library'; ``` ########## superset-frontend/src/components/Table/utils/InteractiveTableUtils.ts: ########## @@ -0,0 +1,237 @@ +/** + * 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 type { ColumnsType } from 'antd/es/table'; +import { SUPERSET_TABLE_COLUMN } from 'src/components/Table'; +import { withinRange } from './utils'; + +interface IInteractiveColumn extends HTMLElement { + mouseDown: boolean; + oldX: number; + oldWidth: number; + draggable: boolean; +} +export default class InteractiveTableUtils { + tableRef: HTMLTableElement | null; + + columnRef: IInteractiveColumn | null; + + setDerivedColumns: Function; + + isDragging: boolean; + + resizable: boolean; + + reorderable: boolean; + + derivedColumns: ColumnsType<any>; + + RESIZE_INDICATOR_THRESHOLD: number; + + constructor( + tableRef: HTMLTableElement, + derivedColumns: ColumnsType<any>, + setDerivedColumns: Function, + ) { + this.setDerivedColumns = setDerivedColumns; + this.tableRef = tableRef; + this.isDragging = false; + this.RESIZE_INDICATOR_THRESHOLD = 8; + this.resizable = false; + this.reorderable = false; + this.derivedColumns = [...derivedColumns]; + document.addEventListener('mouseup', this.handleMouseup); + } + + clearListeners = () => { + document.removeEventListener('mouseup', this.handleMouseup); + this.initializeResizableColumns(false, this.tableRef); + this.initializeDragDropColumns(false, this.tableRef); + }; + + setTableRef = (table: HTMLTableElement) => { + this.tableRef = table; + }; + + getColumnIndex = (): number => { + let index = -1; + const parent: HTMLElement | null | undefined = this.columnRef + ?.parentNode as HTMLElement; + if (parent) { + index = Array.prototype.indexOf.call(parent.children, this.columnRef); + } + return index; + }; + + handleColumnDragStart = (ev: DragEvent): void => { + const target: IInteractiveColumn = ev?.currentTarget as IInteractiveColumn; Review Comment: ```suggestion const target = ev?.currentTarget as IInteractiveColumn; ``` ########## superset-frontend/src/components/Table/sorters.ts: ########## @@ -0,0 +1,36 @@ +/** + * 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. + */ + +/** + * @param key The name of the row's attribute used to compare values for alphabetical sorting + * @param a First row object to compare + * @param b Second row object to compare + * @returns boolean + */ +export const alphabeticalSort = (key: string, a: object, b: object) => + a?.[key]?.localeCompare?.(b?.[key]); + +/** + * @param key The name of the row's attribute used to compare values for numerical sorting + * @param a First row object to compare + * @param b Second row object to compare + * @returns boolean + */ +export const numericalSort = (key: string, a: object, b: object) => Review Comment: The returns annotation is not matching the typescript return type. ########## superset-frontend/src/components/Table/utils/InteractiveTableUtils.ts: ########## @@ -0,0 +1,237 @@ +/** + * 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 type { ColumnsType } from 'antd/es/table'; +import { SUPERSET_TABLE_COLUMN } from 'src/components/Table'; +import { withinRange } from './utils'; + +interface IInteractiveColumn extends HTMLElement { + mouseDown: boolean; + oldX: number; + oldWidth: number; + draggable: boolean; +} +export default class InteractiveTableUtils { + tableRef: HTMLTableElement | null; + + columnRef: IInteractiveColumn | null; + + setDerivedColumns: Function; + + isDragging: boolean; + + resizable: boolean; + + reorderable: boolean; + + derivedColumns: ColumnsType<any>; + + RESIZE_INDICATOR_THRESHOLD: number; + + constructor( + tableRef: HTMLTableElement, + derivedColumns: ColumnsType<any>, + setDerivedColumns: Function, + ) { + this.setDerivedColumns = setDerivedColumns; + this.tableRef = tableRef; + this.isDragging = false; + this.RESIZE_INDICATOR_THRESHOLD = 8; + this.resizable = false; + this.reorderable = false; + this.derivedColumns = [...derivedColumns]; + document.addEventListener('mouseup', this.handleMouseup); + } + + clearListeners = () => { + document.removeEventListener('mouseup', this.handleMouseup); + this.initializeResizableColumns(false, this.tableRef); + this.initializeDragDropColumns(false, this.tableRef); + }; + + setTableRef = (table: HTMLTableElement) => { + this.tableRef = table; + }; + + getColumnIndex = (): number => { + let index = -1; + const parent: HTMLElement | null | undefined = this.columnRef + ?.parentNode as HTMLElement; + if (parent) { + index = Array.prototype.indexOf.call(parent.children, this.columnRef); + } + return index; + }; + + handleColumnDragStart = (ev: DragEvent): void => { + const target: IInteractiveColumn = ev?.currentTarget as IInteractiveColumn; + if (target) { + this.columnRef = target; + } + this.isDragging = true; + const index = this.getColumnIndex(); + const columnData = this.derivedColumns[index]; + const dragData = { index, columnData }; + ev?.dataTransfer?.setData(SUPERSET_TABLE_COLUMN, JSON.stringify(dragData)); + }; + + handleDragDrop = (ev: DragEvent): void => { + const data = ev.dataTransfer?.getData?.(SUPERSET_TABLE_COLUMN); + if (data) { + ev.preventDefault(); + const parent: Element = (ev.currentTarget as HTMLElement) + ?.parentNode as HTMLElement; + const dropIndex = Array.prototype.indexOf.call( + parent.children, + ev.currentTarget, + ); + const dragIndex = this.getColumnIndex(); + const columnsCopy = [...this.derivedColumns]; + const removedItem = columnsCopy.slice(dragIndex, dragIndex + 1); + columnsCopy.splice(dragIndex, 1); + columnsCopy.splice(dropIndex, 0, removedItem[0]); + this.derivedColumns = [...columnsCopy]; + this.setDerivedColumns(columnsCopy); + } + }; + + allowDrop = (ev: DragEvent): void => { + ev.preventDefault(); + }; + + handleMouseDown = (event: MouseEvent) => { + const target: IInteractiveColumn = + event?.currentTarget as IInteractiveColumn; Review Comment: ```suggestion const target = event?.currentTarget as IInteractiveColumn; ``` ########## superset-frontend/src/components/Table/sorters.ts: ########## @@ -0,0 +1,36 @@ +/** + * 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. + */ + +/** + * @param key The name of the row's attribute used to compare values for alphabetical sorting + * @param a First row object to compare + * @param b Second row object to compare + * @returns boolean + */ +export const alphabeticalSort = (key: string, a: object, b: object) => + a?.[key]?.localeCompare?.(b?.[key]); Review Comment: The returns annotation is not matching the typescript return type. ########## superset-frontend/src/components/Table/utils/InteractiveTableUtils.ts: ########## @@ -0,0 +1,237 @@ +/** + * 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 type { ColumnsType } from 'antd/es/table'; +import { SUPERSET_TABLE_COLUMN } from 'src/components/Table'; +import { withinRange } from './utils'; + +interface IInteractiveColumn extends HTMLElement { + mouseDown: boolean; + oldX: number; + oldWidth: number; + draggable: boolean; +} +export default class InteractiveTableUtils { + tableRef: HTMLTableElement | null; + + columnRef: IInteractiveColumn | null; + + setDerivedColumns: Function; + + isDragging: boolean; + + resizable: boolean; + + reorderable: boolean; + + derivedColumns: ColumnsType<any>; + + RESIZE_INDICATOR_THRESHOLD: number; + + constructor( + tableRef: HTMLTableElement, + derivedColumns: ColumnsType<any>, + setDerivedColumns: Function, + ) { + this.setDerivedColumns = setDerivedColumns; + this.tableRef = tableRef; + this.isDragging = false; + this.RESIZE_INDICATOR_THRESHOLD = 8; + this.resizable = false; + this.reorderable = false; + this.derivedColumns = [...derivedColumns]; + document.addEventListener('mouseup', this.handleMouseup); + } + + clearListeners = () => { + document.removeEventListener('mouseup', this.handleMouseup); + this.initializeResizableColumns(false, this.tableRef); + this.initializeDragDropColumns(false, this.tableRef); + }; + + setTableRef = (table: HTMLTableElement) => { + this.tableRef = table; + }; + + getColumnIndex = (): number => { + let index = -1; + const parent: HTMLElement | null | undefined = this.columnRef + ?.parentNode as HTMLElement; + if (parent) { + index = Array.prototype.indexOf.call(parent.children, this.columnRef); + } + return index; + }; + + handleColumnDragStart = (ev: DragEvent): void => { + const target: IInteractiveColumn = ev?.currentTarget as IInteractiveColumn; + if (target) { + this.columnRef = target; + } + this.isDragging = true; + const index = this.getColumnIndex(); + const columnData = this.derivedColumns[index]; + const dragData = { index, columnData }; + ev?.dataTransfer?.setData(SUPERSET_TABLE_COLUMN, JSON.stringify(dragData)); + }; + + handleDragDrop = (ev: DragEvent): void => { + const data = ev.dataTransfer?.getData?.(SUPERSET_TABLE_COLUMN); + if (data) { + ev.preventDefault(); + const parent: Element = (ev.currentTarget as HTMLElement) Review Comment: ```suggestion const parent = (ev.currentTarget as HTMLElement) ``` ########## superset-frontend/src/components/Table/utils/InteractiveTableUtils.ts: ########## @@ -0,0 +1,237 @@ +/** + * 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 type { ColumnsType } from 'antd/es/table'; +import { SUPERSET_TABLE_COLUMN } from 'src/components/Table'; +import { withinRange } from './utils'; + +interface IInteractiveColumn extends HTMLElement { + mouseDown: boolean; + oldX: number; + oldWidth: number; + draggable: boolean; +} +export default class InteractiveTableUtils { + tableRef: HTMLTableElement | null; + + columnRef: IInteractiveColumn | null; + + setDerivedColumns: Function; + + isDragging: boolean; + + resizable: boolean; + + reorderable: boolean; + + derivedColumns: ColumnsType<any>; + + RESIZE_INDICATOR_THRESHOLD: number; + + constructor( + tableRef: HTMLTableElement, + derivedColumns: ColumnsType<any>, + setDerivedColumns: Function, + ) { + this.setDerivedColumns = setDerivedColumns; + this.tableRef = tableRef; + this.isDragging = false; + this.RESIZE_INDICATOR_THRESHOLD = 8; + this.resizable = false; + this.reorderable = false; + this.derivedColumns = [...derivedColumns]; + document.addEventListener('mouseup', this.handleMouseup); + } + + clearListeners = () => { + document.removeEventListener('mouseup', this.handleMouseup); + this.initializeResizableColumns(false, this.tableRef); + this.initializeDragDropColumns(false, this.tableRef); + }; + + setTableRef = (table: HTMLTableElement) => { + this.tableRef = table; + }; + + getColumnIndex = (): number => { + let index = -1; + const parent: HTMLElement | null | undefined = this.columnRef + ?.parentNode as HTMLElement; Review Comment: ```suggestion const parent = this.columnRef?.parentNode; ``` ########## superset-frontend/src/components/Table/utils/utils.ts: ########## @@ -0,0 +1,40 @@ +/** + * 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. + */ + +/** + * Method to check if a number is within inclusive range between a maximum value minus a threshold + * Invalid non numeric inputs will not error, but will return false + * + * @param value number coordinate to determine if it is within bounds of the targetCoordinate - threshold. Must be positive and less than maximum. + * @param maximum number max value for the test range. Must be positive and greater than value + * @param threshold number values to determine a range from maximum - threshold. Must be positive and greater than zero. + * @returns Review Comment: Can you document the return here? ########## superset-frontend/src/components/Table/utils/utils.test.ts: ########## @@ -0,0 +1,46 @@ +/** + * 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 { withinRange } from './utils'; + +test('withinRange', () => { Review Comment: Maybe split this into multiple test cases? - test('ranges', ...) - test('negative numbers', ...) - test('invalid input types', ...) ########## superset-frontend/src/components/Table/utils/InteractiveTableUtils.ts: ########## @@ -0,0 +1,237 @@ +/** + * 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 type { ColumnsType } from 'antd/es/table'; +import { SUPERSET_TABLE_COLUMN } from 'src/components/Table'; +import { withinRange } from './utils'; + +interface IInteractiveColumn extends HTMLElement { + mouseDown: boolean; + oldX: number; + oldWidth: number; + draggable: boolean; +} +export default class InteractiveTableUtils { + tableRef: HTMLTableElement | null; + + columnRef: IInteractiveColumn | null; + + setDerivedColumns: Function; + + isDragging: boolean; + + resizable: boolean; + + reorderable: boolean; + + derivedColumns: ColumnsType<any>; + + RESIZE_INDICATOR_THRESHOLD: number; + + constructor( + tableRef: HTMLTableElement, + derivedColumns: ColumnsType<any>, + setDerivedColumns: Function, + ) { + this.setDerivedColumns = setDerivedColumns; + this.tableRef = tableRef; + this.isDragging = false; + this.RESIZE_INDICATOR_THRESHOLD = 8; + this.resizable = false; + this.reorderable = false; + this.derivedColumns = [...derivedColumns]; + document.addEventListener('mouseup', this.handleMouseup); + } + + clearListeners = () => { + document.removeEventListener('mouseup', this.handleMouseup); + this.initializeResizableColumns(false, this.tableRef); + this.initializeDragDropColumns(false, this.tableRef); + }; + + setTableRef = (table: HTMLTableElement) => { + this.tableRef = table; + }; + + getColumnIndex = (): number => { + let index = -1; + const parent: HTMLElement | null | undefined = this.columnRef + ?.parentNode as HTMLElement; + if (parent) { + index = Array.prototype.indexOf.call(parent.children, this.columnRef); + } + return index; + }; + + handleColumnDragStart = (ev: DragEvent): void => { + const target: IInteractiveColumn = ev?.currentTarget as IInteractiveColumn; + if (target) { + this.columnRef = target; + } + this.isDragging = true; + const index = this.getColumnIndex(); + const columnData = this.derivedColumns[index]; + const dragData = { index, columnData }; + ev?.dataTransfer?.setData(SUPERSET_TABLE_COLUMN, JSON.stringify(dragData)); + }; + + handleDragDrop = (ev: DragEvent): void => { + const data = ev.dataTransfer?.getData?.(SUPERSET_TABLE_COLUMN); + if (data) { + ev.preventDefault(); + const parent: Element = (ev.currentTarget as HTMLElement) + ?.parentNode as HTMLElement; + const dropIndex = Array.prototype.indexOf.call( + parent.children, + ev.currentTarget, + ); + const dragIndex = this.getColumnIndex(); + const columnsCopy = [...this.derivedColumns]; + const removedItem = columnsCopy.slice(dragIndex, dragIndex + 1); + columnsCopy.splice(dragIndex, 1); + columnsCopy.splice(dropIndex, 0, removedItem[0]); + this.derivedColumns = [...columnsCopy]; + this.setDerivedColumns(columnsCopy); + } + }; + + allowDrop = (ev: DragEvent): void => { + ev.preventDefault(); + }; + + handleMouseDown = (event: MouseEvent) => { + const target: IInteractiveColumn = + event?.currentTarget as IInteractiveColumn; + if (target) { + this.columnRef = target; + if ( + event && + withinRange( + event.offsetX, + target.offsetWidth, + this.RESIZE_INDICATOR_THRESHOLD, + ) + ) { + target.mouseDown = true; + target.oldX = event.x; + target.oldWidth = target.offsetWidth; + target.draggable = false; + } else if (this.reorderable) { + target.draggable = true; + } + } + }; + + handleMouseMove = (event: MouseEvent) => { + if (this.resizable === true && !this.isDragging) { + const target: IInteractiveColumn = + event.currentTarget as IInteractiveColumn; Review Comment: ```suggestion const target = event.currentTarget as IInteractiveColumn; ``` ########## superset-frontend/src/components/Table/sorters.test.ts: ########## @@ -0,0 +1,103 @@ +/** + * 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 { alphabeticalSort, numericalSort } from './sorters'; + +const rows = [ + { + name: 'Deathstar Lamp', + category: 'Lamp', + cost: 75.99, + }, + { + name: 'Desk Lamp', + category: 'Lamp', + cost: 15.99, + }, + { + name: 'Bedside Lamp', + category: 'Lamp', + cost: 15.99, + }, + { name: 'Drafting Desk', category: 'Desk', cost: 125 }, + { name: 'Sit / Stand Desk', category: 'Desk', cost: 275.99 }, +]; + +/** + * NOTE: Sorters for antd table use < 0, 0, > 0 for sorting + * -1 or less means the first item comes after the second item + * 0 means the items sort values is equivalent + * 1 or greater means the first item comes before the second item + */ +test('alphabeticalSort sorts correctly', () => { + expect(alphabeticalSort('name', rows[0], rows[1])).toBe(-1); + expect(alphabeticalSort('name', rows[1], rows[0])).toBe(1); + expect(alphabeticalSort('category', rows[1], rows[0])).toBe(0); +}); + +test('numericalSort sorts correctly', () => { + expect(numericalSort('cost', rows[1], rows[2])).toBe(0); + expect(numericalSort('cost', rows[1], rows[0])).toBeLessThan(0); + expect(numericalSort('cost', rows[4], rows[1])).toBeGreaterThan(0); +}); + +/** + * We want to make sure our sorters do not throw runtime errors given bad inputs. + * Runtime Errors in a sorter will cause a catastrophic React lifecycle error and produce white screen of death + * In the case the sorter cannot perform the comparison it should return undefined and the next sort step will proceed without error + */ +test('alphabeticalSort bad inputs no errors', () => { + // @ts-ignore + expect(alphabeticalSort('name', null, null)).toBe(undefined); + // incorrect non-object values + // @ts-ignore + expect(alphabeticalSort('name', 3, [])).toBe(undefined); + // incorrect object values without specificed key + // @ts-ignore + expect(alphabeticalSort('name', {}, {})).toBe(undefined); + // Object as value for name when it should be a string + // @ts-ignore Review Comment: ```suggestion ``` -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
