Skip to content

Conversation

@artemmufazalov
Copy link
Member

@artemmufazalov artemmufazalov commented Feb 1, 2024

Add resize for "Version" column in Nodes table - little native resize triangle near right bottom corner.

Screen Shot 2024-02-02 at 11 10 41

The table could be reviewed in UI here: https://nda.ya.ru/t/aL9co3Y674WeyY

To enable VirtualTable turn on "Use table with data load on scroll for Nodes and Storage tabs " experiment in settings.

return <CellWithPopover content={row.Version}>{row.Version}</CellWithPopover>;
},
sortable: false,
resizeable: true,
Copy link
Member Author

@artemmufazalov artemmufazalov Feb 1, 2024

Choose a reason for hiding this comment

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

Currently only Version column is resizeable, because proper resize also requires update for components used inside cells.

For example, NodesHostWrapper has fixed 330px width, resize makes no sense for such component.

Other columns will be added further step by step, if needed

@artemmufazalov artemmufazalov marked this pull request as ready for review February 2, 2024 08:13
defaultSortOrder={defaultSortOrder}
onSort={handleSort}
rowHeight={rowHeight}
resizeObserver={resizeObserver.current}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems it would be better not to pass resizeObserver, but make a component with forwardRef and do all observe staff here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I intentionally created TableHeadCell component to observe and unobserve <div> on mount and unmount in useEffect. The reason - it's very important to correctly unobserve <div> on unmount, otherwise some odd values could be stored as column width.

In case with forwardRef, it's needed to store some reference to initial divs, to manually unobserve them on their unmount. So with removing useEffect construction from HeadCell some more complex construction will appear in TableHead

Copy link
Contributor

Choose a reason for hiding this comment

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

Nonetheless, it's not correct to pass ref.current as property, because React doesn't guarantee to rerender children if ref.current will change.
So, as for me, array with refs is better solution. You may pass unsubscribe method to all ths.

Copy link
Member Author

Choose a reason for hiding this comment

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

Always forget about this issue with useRef.

Replaced it with useState, now it should rerender properly.

As for the array of refs option, it won't be just array of refs, because there are cases, where columns could be unmounted one by one. Examples: tables with conditional columns - Storage, where degraded columns is shown only with specific filters setup, Partitions, where some columns appear, if consumer is selected; tables with columns setup - you can hide and show specific columns. So it either would be some additional events handlers, or some logic, to process this array to subscribe new columns and unsubscribe disappeared.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we create functions in TableHead and pass it to all cells? Or this logic may be moved to custom hook.

const observedCells = React.useRef({})

const {observeCell, unobserveCell} = React.useMemo(() => {
  const resizeObserver = new ResizeObserver(......)

  const unobserveCell = (cellId) => {
  const cellRef = observedCells.current.cellId
  if (cellRef) {
    resizeObserver.unobserve(cellRef)
    delete observedCells.current.cellId
  }
}
const observeCell = (cellId, cellRef) => {
  unobserveCell(cellId)
  resizeObserver.unobserve(cellRef)
  observedCells.current.cellId = cellRef
}

  return {observeCell, unobserveCell}

}, [onColumnsResize, isTableResizeable])

@sareyu
Copy link
Collaborator

sareyu commented Feb 2, 2024

image

add min-width for column and overflow: ellipsys for cells plz

sareyu
sareyu previously approved these changes Feb 2, 2024
align = DEFAULT_ALIGN,
}: TableCellProps) => {
// Additional wrapper <div> with explicit width to ensure proper overflow:hidden
// since overflow works poorly with <td>
Copy link
Collaborator

Choose a reason for hiding this comment

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

overflow works only with block elements, afaik
try this hack with max-width plz to avoid extra element, looks like it suits our scenario

position: absolute;
top: 50%;
right: 0;
&__row-cell {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not cell, it's cell-content

NodesUptimeFilterValues.All,
);

const [tableColumnsWidthSetup, setTableColumnsWidth] = useTableResize('nodesTableColumnsWidth');
Copy link
Collaborator

Choose a reason for hiding this comment

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

you read resized width only once so maybe you can split this hook in two to avoid passing onColumnsResize callback through two levels (minor, don`t insist)

Copy link
Member Author

Choose a reason for hiding this comment

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

I created it as a hook (not some prop inside VirtualTable) to reuse it with react-data-table (I assume onTableResize will be a callback from outside)

defaultSortOrder={defaultSortOrder}
onSort={handleSort}
rowHeight={rowHeight}
resizeObserver={resizeObserver.current}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nonetheless, it's not correct to pass ref.current as property, because React doesn't guarantee to rerender children if ref.current will change.
So, as for me, array with refs is better solution. You may pass unsubscribe method to all ths.

@artemmufazalov artemmufazalov merged commit 907b275 into main Feb 7, 2024
@artemmufazalov artemmufazalov deleted the table-resize branch February 7, 2024 15:03
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.

Resizeable columns for VirtualTable with css resize

4 participants