Skip to content

feat: add columns resize#41

Merged
artemmufazalov merged 1 commit intomainfrom
feat/add-columns-resize
Apr 12, 2024
Merged

feat: add columns resize#41
artemmufazalov merged 1 commit intomainfrom
feat/add-columns-resize

Conversation

@artemmufazalov
Copy link
Copy Markdown
Contributor

No description provided.

@gravity-ui-bot
Copy link
Copy Markdown
Contributor

Preview is ready.

@artemmufazalov artemmufazalov force-pushed the feat/add-columns-resize branch 5 times, most recently from fd39320 to 64dac3e Compare April 10, 2024 10:08
Comment thread README.md Outdated
Comment on lines +95 to +97
| resizeable     | `boolean` | Determines whether column width can be changed. Applied only if numeric `width` is set and `onResize` function is passed. |
| resizeMinWidth | `number` | Min column width for resize. |
| resizeMaxWidth | `number` | Max column width for resize. |
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added values

@artemmufazalov artemmufazalov force-pushed the feat/add-columns-resize branch from 64dac3e to 749e44a Compare April 10, 2024 10:29
@artemmufazalov artemmufazalov marked this pull request as ready for review April 10, 2024 10:33
Comment thread src/lib/DataTable.tsx Outdated
{header}
{<ColumnSortIcon {...column} />}
</div>
{this.renderCellResizeHandler(column)}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maybe use same approach as for ColumnSortIcon?
<ResizeHandler {...column} onResize={onResize} />

Copy link
Copy Markdown
Contributor Author

@artemmufazalov artemmufazalov Apr 10, 2024

Choose a reason for hiding this comment

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

I wanted to check whether it's needed and return null separately, so there won't be additional logic inside ResizeHandler itself

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can add conditional render here. in method you rewrite propnames, write own type instead of using column's type and method are usually used in class components approach, not functional. not a mistake, just a bit awkward

Comment thread src/stories/Resizeable/Resizeable.tsx Outdated
];

export function ResizeableTable({theme = YCLOUD_THEME, ...props}: DataTableProps<RowType>) {
const [tableColumnsWidthSetup, setTableColumnsWidth] = useTableResize('resizeableTable');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

does this approach will cause rerender whole table for each resize move? this may cause performance problems for big tables, maybe try to apply native resize solution, when resize affects only placeholder in header which affects whole column width?

Copy link
Copy Markdown
Contributor Author

@artemmufazalov artemmufazalov Apr 10, 2024

Choose a reason for hiding this comment

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

I didn't figured out how to resize only a single column in current realisation. It seems, everywhere it renders like columns.map(renderFunction), so I cannot just update one column. And also width should be updated in many places at once (column, colgroup, rows, cells).

As for the native resize, it assumes defined resize icon, which I cannot change. Also native resize requires some css and html refactoring. And it will also cause whole table rerender, since we need to update not only column, but also colgroup and cells (for proper content wrap). I applied solution, similar to default resize handler, that we have in internal common-datagrid, it looks much better

Comment thread src/lib/DataTable.tsx Outdated
if (column.resizeable) {
style = style
? {...style, width: column.width, maxWidth: column.width}
: {width: column.width, maxWidth: column.width};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we realy need to checkstyle?

Comment thread src/lib/DataTable.tsx Outdated
if (column.resizeable) {
style = style
? {...style, width: column.width, maxWidth: column.width}
: {width: column.width, maxWidth: column.width};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we realy need to checkstyle? It should work fine:

style = {...style, width: column.width, maxWidth: column.width};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Didn't know js correctly spreads undefined)

Comment thread src/lib/DataTable.tsx Outdated
renderCellResizeHandler(column: HeadCellColumn<T>) {
const {onResize} = this.props;

if (!column.resizeable || !onResize || typeof column.width !== 'number') {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there reasons to not use the condition to check if need to apply width/maxWidth to style of td/th?
I.e. why do we check only column.resizeable in line 135/266?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I set correct resizeable property in getColumn method (it also applies default value): https://github.com/gravity-ui/react-data-table/blob/feat/add-columns-resize/src/lib/DataTable.tsx#L1147

So check if (!column.resizeable) should be enough in here and everywhere else. However, final resizeable property on column doesn't implies actual onResize and width types. So this check here to not add additional logic to ResizeHandler and to not use type assertions.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is it possible to resize a column without explicitly defined width property?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. If width isn't set explicitly, it will work as before - column width corresponds to content width

Comment thread src/lib/useTableResize.ts Outdated

import type {Column, HandleResize} from './types';

export type TableColumnsWidthSetup = Record<string, number>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

May be it is better to name it ColumnWidthByName/WidthByColname /WidthByName (not an issue).

Comment thread src/lib/useTableResize.ts Outdated
});
}

export function useTableResize(localStorageKey: string): [TableColumnsWidthSetup, HandleResize] {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Current implementation promotes to use localStorage of browser and it doesn't looks like a good idea cause it has 5MB limit for stored data. And some services with many tables may overflow the limit in some time. It is better to require some abstract methods in parameters of the hook:

function useTableResize(
    params: {
        saveSizes: (data: TableColumnsWidthSetup) => void;
        getSizes: () => TableColumnsWidthSetup;
    }
): [TableColumnsWidthSetup, HandleResize] {
    ...
}```

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I assumed this hook as basic implementation with the possibility to write your own hook if needed (that is why it is a separate hook, but not some logic inside DataTable). But your solution event better, I'll use it

@artemmufazalov artemmufazalov force-pushed the feat/add-columns-resize branch from 6f288b2 to b9f719d Compare April 12, 2024 13:27
@artemmufazalov artemmufazalov force-pushed the feat/add-columns-resize branch from b9f719d to 54330ce Compare April 12, 2024 13:34
@artemmufazalov artemmufazalov merged commit 2d3f79f into main Apr 12, 2024
@artemmufazalov artemmufazalov deleted the feat/add-columns-resize branch April 12, 2024 13:36
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.

4 participants