Skip to content

Conversation

kyle-marcum
Copy link
Contributor

No description provided.

@kyle-marcum kyle-marcum force-pushed the fe/feature/RI-5150_statistics-page branch 3 times, most recently from eef18e0 to d90ca20 Compare March 1, 2024 15:40
actions: JSX.Element
}

const Header = ({ actions }: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you take a look at this component? RdiPipelineHeader i guess we can combine them in one component

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 will take a look


useEffect(() => {
dispatch(fetchRdiPipeline(rdiInstanceId))
if (!pipelineData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe will better move dispatch(fetchRdiPipeline(rdiInstanceId)) to RdiInstancePage ?

content="My RDI instances"
<Header
actions={(
<EuiOutsideClickDetector onOutsideClick={handleClosePopover}>
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, i see what you did. We need to think about rdi templates (i global template and one for pipeline management)

}

useEffect(() => {
if (!pipelineData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

probably you can move fetch rdi pipeline and fetchConnectedInstanceAction in Instance page and remove it from here and from pipelineMangementPage, what do you think about it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea

<RdiStatisticsHeader />
<ExplorePanelTemplate>
<div className={styles.bodyContainer}>
{isEmpty(pipelineData) ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not forget to check this behavior please. We still don't know how will look not deployed pipeline

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 will add a TODO so that we don't forget

}
}
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

add blank line, please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


import styles from './styles.module.scss'

const VerticalDivider = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that we really need it. @romansergeenkosofteq What do you think? i guess we can avoid additional component

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'm good with using the Divider component directly instead. Just tried to make the usage more succinct.

import InsightsTrigger from 'uiSrc/components/insights-trigger'
import Header from 'uiSrc/pages/rdi/components/header'

const RdiStatisticsHeader = () => <Header actions={<InsightsTrigger source="statistics page" />} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to use ExplorePanelTemplate here? anyway we need to think about templates for rdi pages. But need final mockup for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going to remove ExplorePanelTemplate for now since it will be implemented in a later card

Copy link
Contributor

@AmirAllayarovSofteq AmirAllayarovSofteq left a comment

Choose a reason for hiding this comment

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

Good job.
Need to think about templates. And what do you think about move fetch pipeline and instance info in RDIInstancePage?

@kyle-marcum kyle-marcum force-pushed the fe/feature/RI-5150_statistics-page branch 2 times, most recently from e9fa82c to b68e29a Compare March 6, 2024 15:21
Base automatically changed from be/feature/RI-5150_statistics-page to feature/RI-4616-rdi-support March 6, 2024 17:19
@kyle-marcum kyle-marcum force-pushed the fe/feature/RI-5150_statistics-page branch from 607f4bc to 3d55011 Compare March 6, 2024 17:47
Copy link
Collaborator

@ArtemHoruzhenko ArtemHoruzhenko left a comment

Choose a reason for hiding this comment

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

Lgtm
Fix tests before merge pkease

@kyle-marcum kyle-marcum merged commit ccd3a6b into feature/RI-4616-rdi-support Mar 6, 2024
@kyle-marcum kyle-marcum deleted the fe/feature/RI-5150_statistics-page branch March 6, 2024 19:07
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.

3 participants