Skip to content

Conversation

snowystinger
Copy link
Member

Closes

Found in audit. The function properties of a stately object shouldn't follow the on* pattern. Instead, they should be clear that they are going to do something, not cause a side effect.

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Feb 13, 2023

LFDanLu
LFDanLu previously approved these changes Feb 13, 2023
@rspbot
Copy link

rspbot commented Feb 13, 2023

@rspbot
Copy link

rspbot commented Feb 13, 2023

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

nit

}, [setResizingColumn]);

let onColumnResize = useCallback((key: Key, width: number): Map<Key, ColumnSize> => {
let computeResizedColumns = useCallback((key: Key, width: number): Map<Key, ColumnSize> => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe updateResizedColumns would be slightly better because this does have a side effect of updating the uncontrolled widths in addition to just computing the sizes...

@rspbot
Copy link

rspbot commented Feb 15, 2023

@rspbot
Copy link

rspbot commented Feb 15, 2023

Copy link
Member

@ktabors ktabors left a comment

Choose a reason for hiding this comment

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

I like the new names and resizing still works.

@rspbot
Copy link

rspbot commented Feb 15, 2023

@rspbot
Copy link

rspbot commented Feb 15, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

@react-stately/layout

TableLayout

 TableLayout<T> {
   addVisibleLayoutInfos: (Array<LayoutInfo>, LayoutNode, Rect) => void
   binarySearch: (Array<LayoutNode>, Point, 'x' | 'y') => void
   buildBody: (number) => LayoutNode
   buildCell: (GridNode<T>, number, number) => LayoutNode
   buildCollection: () => Array<LayoutNode>
   buildColumn: (GridNode<T>, number, number) => LayoutNode
   buildHeader: () => LayoutNode
   buildHeaderRow: (GridNode<T>, number, number) => LayoutNode
   buildNode: (GridNode<T>, number, number) => LayoutNode
   buildPersistedIndices: () => void
   buildRow: (GridNode<T>, number, number) => LayoutNode
   collection: TableCollection<T>
   columnLayout: TableColumnLayout<T>
   columnWidths: Map<Key, number>
   constructor: (TableLayoutOptions<T>) => void
   controlledColumns: Map<Key, GridNode<unknown>>
+  endResize: () => void
   getColumnMaxWidth: (Key) => number
   getColumnMinWidth: (Key) => number
   getColumnWidth: (Key) => number
   getEstimatedHeight: (GridNode<T>, number, number, number) => void
   getInitialLayoutInfo: (LayoutInfo) => void
   getRenderedColumnWidth: (GridNode<T>) => void
   getResizerPosition: () => Key
   getVisibleLayoutInfos: (Rect) => void
   isLoading: any
   lastCollection: TableCollection<T>
   lastPersistedKeys: Set<Key>
-  onColumnResize: (Key, number) => Map<Key, ColumnSize>
-  onColumnResizeEnd: () => void
-  onColumnResizeStart: (Key) => void
   persistedIndices: Map<Key, Array<number>>
   resizingColumn: Key | null
   setChildHeights: (Array<LayoutNode>, number) => void
+  startResize: (Key) => void
   stickyColumnIndices: Array<number>
   uncontrolledColumns: Map<Key, GridNode<unknown>>
   uncontrolledWidths: Map<Key, ColumnSize>
+  updateResizedColumns: (Key, number) => Map<Key, ColumnSize>
   wasLoading: any
 }

@react-stately/table

TableColumnResizeState

 TableColumnResizeState<T> {
+  endResize: () => void
   getColumnMaxWidth: (Key) => number
   getColumnMinWidth: (Key) => number
   getColumnWidth: (Key) => number
-  onColumnResize: (Key, number) => Map<Key, ColumnSize>
-  onColumnResizeEnd: () => void
-  onColumnResizeStart: (Key) => void
   resizingColumn: Key | null
+  startResize: (Key) => void
   tableState: TableState<T>
+  updateResizedColumns: (Key, number) => Map<Key, ColumnSize>
   widths: Map<Key, number>
 }

it changed:

  • useTableColumnResizeState

@snowystinger snowystinger merged commit b075dc5 into main Feb 15, 2023
@snowystinger snowystinger deleted the update-stately-names branch February 15, 2023 00:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants