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

Custom pipeline groups #5

Closed
wants to merge 18 commits into from
Closed

Custom pipeline groups #5

wants to merge 18 commits into from

Conversation

jenny-s51
Copy link
Owner

Adds subgraph support, collapsible groups, scaled group nodes, hoverable logic

https://issues.redhat.com/browse/RHOAIENG-3977

@jenny-s51 jenny-s51 marked this pull request as draft April 12, 2024 19:14
jenny-s51 and others added 11 commits April 14, 2024 18:07
remove unused params

translate artifact node

add Jeff fixes

expose monitoring icon for metric artifacts

Artifact node fixes

add groups logic to render collapsed group node styling

remove groups logic, fix icon color

remove PipelineTaskGroup

remove unused TaskGroupTargetAnchor

fix artifact node dimensions

fix linting issues

add support for nested pipeline groups, wip

run --fix

add status to artifact nodes and fix hovered/selected icon color

delete IconTaskNode

remove custom-topology-components.css

use forEach

revert tsconfig to es5
const childCount = element.getAllNodeChildren().length;
const data = element.getData();

const handleCollapse = action((group: Node, collapsed: boolean): void => {

Choose a reason for hiding this comment

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

This will get done in the DefaultTaskGroup for expanded groups. Remove this here and move it to PipelineTaskGroupCollapsed.

}
});

if (element.isCollapsed()) {

Choose a reason for hiding this comment

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

Add a comment that this is temporary until patternfly/react-topology#171 is completed.

Copy link
Owner Author

Choose a reason for hiding this comment

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

PR with comment to clarify current collapsible support: patternfly/react-topology#176

labelPosition?: LabelPosition; // Defaults to bottom
badge?: string;
} & CollapsibleGroupProps &
WithSelectionProps;

Choose a reason for hiding this comment

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

Only take the props that are being passed, not need to take those that are never being set.

@@ -0,0 +1,19 @@
import { AbstractAnchor, Point, Node } from '@patternfly/react-topology';

export default class IconSourceAnchor<E extends Node = Node> extends AbstractAnchor {

Choose a reason for hiding this comment

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

Is this being used?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Nope, nice catch - removed this file


getReferencePoint(): Point {
const bounds = this.owner.getBounds();
return new Point(bounds.x + this.size, bounds.y + bounds.height / 2);

Choose a reason for hiding this comment

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

This would only work for horizontal layouts

return withPanZoom()(GraphComponent);
}
switch (type) {
case 'Execution':

Choose a reason for hiding this comment

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

constant?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated

@jenny-s51 jenny-s51 marked this pull request as ready for review April 17, 2024 19:33
@jenny-s51 jenny-s51 changed the title [WIP] Custom pipeline groups Custom pipeline groups Apr 17, 2024
]}
collapsedHeight={NODE_HEIGHT}
collapsedWidth={NODE_WIDTH}
{...rest}

Choose a reason for hiding this comment

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

No need for this

Suggested change
{...rest}

};

const DefaultTaskGroupInner: React.FunctionComponent<PipelinesDefaultGroupInnerProps> = observer(
({ element, onCollapseChange, selected, onSelect, ...rest }) => {

Choose a reason for hiding this comment

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

Suggested change
({ element, onCollapseChange, selected, onSelect, ...rest }) => {
({ element, onCollapseChange, selected, onSelect }) => {

Comment on lines +30 to +34
showStatusState?: boolean;
status?: RunStatus;
hiddenDetailsShownStatuses?: RunStatus[];
hideDetailsAtMedium?: boolean;
onCollapseChange?: (group: Node, collapsed: boolean) => void;

Choose a reason for hiding this comment

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

Suggested change
showStatusState?: boolean;
status?: RunStatus;
hiddenDetailsShownStatuses?: RunStatus[];
hideDetailsAtMedium?: boolean;
onCollapseChange?: (group: Node, collapsed: boolean) => void;

None of these are ever passed

import { getNodeStatusIcon } from './utils';

type PipelinesDefaultGroupProps = {
children?: React.ReactNode;

Choose a reason for hiding this comment

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

Suggested change
children?: React.ReactNode;

visualizationController.registerComponentFactory(pipelineComponentFactory);
visualizationController.registerComponentFactory(pipelineGroupsComponentFactory);

Choose a reason for hiding this comment

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

If you are including pipelineComponentFactory then pipelineGroupsComponentFactory does not need to duplicate the types in its switch statement. PF will check all factories in reverse order added, so only overrides need to be in the later factories.

If you add the PF pipelines factory (first), you could also remove the DEFAULT_SPACER_NODE_TYPE and DEFAULT_EDGE_TYPE cases.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great point, thank you @jeff-phillips-18 . I've addressed this comment and your suggestions above in opendatahub-io#2725

@jenny-s51 jenny-s51 closed this Apr 26, 2024
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.

2 participants