-
Notifications
You must be signed in to change notification settings - Fork 98
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
Conversation
Preview is ready. |
Playwright Test Component is ready. |
faece26
to
21df027
Compare
export interface WithTableActionsProps<I> { | ||
getRowActions: (item: I, index: number) => TableActionConfig<I>[]; | ||
getRowActions?: (item: I, index: number) => TableActionConfig<I>[]; | ||
renderRowActions?: (props: RenderRowActionsProps<I>) => React.ReactNode; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is for passing React components instead simple functions
@@ -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'; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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>({ |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#311