Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add columns resize #41

Merged
merged 1 commit into from
Apr 12, 2024
Merged

feat: add columns resize #41

merged 1 commit into from
Apr 12, 2024

Conversation

artemmufazalov
Copy link
Contributor

No description provided.

@gravity-ui-bot
Copy link
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
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
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 marked this pull request as ready for review April 10, 2024 10:33
onClick={this._getOnSortClick(column)}
>
<div className={b('head-cell')}>
{header}
{<ColumnSortIcon {...column} />}
</div>
{this.renderCellResizeHandler(column)}
Copy link

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
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

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

];

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

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
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

if (column.resizeable) {
style = style
? {...style, width: column.width, maxWidth: column.width}
: {width: column.width, maxWidth: column.width};
Copy link
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?

if (column.resizeable) {
style = style
? {...style, width: column.width, maxWidth: column.width}
: {width: column.width, maxWidth: column.width};
Copy link
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
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)

renderCellResizeHandler(column: HeadCellColumn<T>) {
const {onResize} = this.props;

if (!column.resizeable || !onResize || typeof column.width !== 'number') {
Copy link
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
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
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
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


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

export type TableColumnsWidthSetup = Record<string, number>;
Copy link
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).

});
}

export function useTableResize(localStorageKey: string): [TableColumnsWidthSetup, HandleResize] {
Copy link
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
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 merged commit 2d3f79f into main Apr 12, 2024
3 checks passed
@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