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(Table): add renderRowActions property #1353

Merged
merged 5 commits into from
Feb 16, 2024
Merged

feat(Table): add renderRowActions property #1353

merged 5 commits into from
Feb 16, 2024

Conversation

GermanVor
Copy link
Contributor

@GermanVor GermanVor commented Feb 15, 2024

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@gravity-ui-bot
Copy link
Contributor

Playwright Test Component is ready.

export interface WithTableActionsProps<I> {
getRowActions: (item: I, index: number) => TableActionConfig<I>[];
getRowActions?: (item: I, index: number) => TableActionConfig<I>[];
renderRowActions?: (props: RenderRowActionsProps<I>) => React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

May be same interface as a getRowActions?
getRowActions?: (item: I, index: number) => React.ReactNode;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@korvin89 korvin89 changed the title [WIP] Render Row Actions feat(Table): add renderRowActions property Feb 16, 2024
@@ -3,6 +3,7 @@ import React from 'react';
import {Ellipsis} from '@gravity-ui/icons';
import _memoize from 'lodash/memoize';

import type {PopperPlacement} from '../../../../hooks/private';
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consume this type from Popup index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, it takes from ../hooks/private/index/usePopper/index/usePopper

export type PopperPlacement = popper.Placement | popper.Placement[];

and PopperPlacement is used in PopperProps and PopupProps extends from PopperProps

const b = block('table');
const actionsCn = b('actions');
const BUTTON_CLASS_NAME = b('actions-button');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this variable named in different case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -131,6 +132,27 @@ const WithTableActionsTemplate: StoryFn<TableProps<DataItem>> = (args) => {
};
export const HOCWithTableActions = WithTableActionsTemplate.bind({});

// ---------------------------------
const WithTableActionsRenderRowActionsTemplate: StoryFn<TableProps<DataItem>> = (args) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add this story to HOC with table actions section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it is impossible to combine getRowActions and renderRowActions at the same time.
renderRowActions will override getRowActions

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to! It will be two stories with different props, just like in https://github.com/gravity-ui/uikit/blob/main/src/components/Table/__stories__/Adaptive.tsx .
It will be nice if you render labels for each (in the example above there are no labels and it looks a little messy).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

index: number;
};

const DefaultRenderRowActions = <I extends TableDataItem>({
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a component, so it's name seems to look better like DefaultRowActions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GermanVor GermanVor merged commit e074b1a into main Feb 16, 2024
4 checks passed
@GermanVor GermanVor deleted the renderRowActions branch February 16, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants