Refactor association table in create page #4
Conversation
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
Signed-off-by: Kapian1234 <wanjinch@amazon.com>
| @@ -81,7 +81,7 @@ export interface AssociationDataSourceModalProps { | |||
| savedObjects: SavedObjectsStart; | |||
| assignedConnections: DataSourceConnection[]; | |||
| closeModal: () => void; | |||
| handleAssignDataSourceConnections: (connections: DataSourceConnection[]) => Promise<void>; | |||
| handleAssignDataSourceConnections: (connections: DataSourceConnection[]) => Promise<void> | void; | |||
There was a problem hiding this comment.
From my perspective, can remove Promise<void>?
There was a problem hiding this comment.
const handleAssignDataSourceConnections = async ( in detail page may need this
There was a problem hiding this comment.
You can try to remove it. The type should work fine. Because we doesn't await handleAssignDataSourceConnections inside modal component.
| @@ -48,7 +48,7 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => { | |||
| const disabledUserOrGroupInputIdsRef = useRef( | |||
| defaultValues?.permissionSettings?.map((item) => item.id) ?? [] | |||
| ); | |||
| const isDashboardAdmin = application?.capabilities?.dashboards?.isDashboardAdmin ?? false; | |||
| const isDashboardAdmin = !!application?.capabilities?.dashboards?.isDashboardAdmin; | |||
There was a problem hiding this comment.
it will throw an error "Type 'true | Readonly<{ [x: string]: boolean; }>' is not assignable to type 'boolean'.
Type 'Readonly<{ [x: string]: boolean; }>' is not assignable to type 'boolean'. " if we don't change this
There was a problem hiding this comment.
We don't need to pass it to other component anymore. So I think we can remove this change.
| data-test-subj={`workspaceForm-dataSourcePanel`} | ||
| isDashboardAdmin={isDashboardAdmin} |
There was a problem hiding this comment.
isDashboardAdmin will always be true here, why we need to pass it from outside?
| @@ -49,7 +49,7 @@ export const useWorkspaceForm = ({ | |||
| Array<Pick<WorkspacePermissionSetting, 'id'> & Partial<WorkspacePermissionSetting>> | |||
| >(initialPermissionSettingsRef.current); | |||
|
|
|||
| const [selectedDataSources, setSelectedDataSources] = useState<DataSource[]>( | |||
| const [selectedDataSources, setSelectedDataSources] = useState<DataSourceConnection[]>( | |||
There was a problem hiding this comment.
Can we keep using DataSource[] as data type here?
There was a problem hiding this comment.
DataSourceConnection[] was used in https://github.com/yubonluo/OpenSearch-Dashboards/tree/2.17/support-DQC
There was a problem hiding this comment.
Can't find related changes in that branch, can you paste me the code line by URL?
There was a problem hiding this comment.
My apologies, I made a mistake.
WorkspaceForm would get selectedDataSources and setSelectedDataSources, then pass them to SelectDataSourcePanel. SelectDataSourcePanel would need them
https://github.com/Kapian1234/OpenSearch-Dashboards/blob/f22c7ac958ec5835c44a45e3447bd32d6b29b45b/src/plugins/workspace/public/components/workspace_form/select_data_source_panel.tsx#L28
| handleUnassignDataSources: (dataSources: DataSourceConnection[]) => Promise<void>; | ||
| inCreatePage?: boolean; | ||
| handleUnassignDataSources: (dataSources: DataSourceConnection[]) => Promise<void> | void; | ||
| getSelectedItems?: (dataSources: DataSourceConnection[]) => void; |
There was a problem hiding this comment.
Prefer rename to onSelectedItemsChange
| {isDashboardAdmin && <EuiFlexItem grow={false}>{associationButton}</EuiFlexItem>} | ||
| </EuiFlexGroup> | ||
| <EuiSpacer size="xs" /> | ||
| <EuiFlexItem style={{ maxWidth: 800 }}> |
| const handleAssignDataSources = (dataSources: DataSourceConnection[]) => { | ||
| setModalVisible(false); | ||
| const savedDataSources: DataSourceConnection[] = [...assignedDataSources, ...dataSources]; | ||
| onChange(savedDataSources); |
There was a problem hiding this comment.
By this onChange, the selected DQC won't be displayed. How do we support this?
There was a problem hiding this comment.
the table already supported it by getting relatedConnections from datasources
Description
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integration