feat(Topology): Add Topology pipeline support #7609

jeff-phillips-18 · 2022-06-24T12:34:16Z

What:
Create a layout and default nodes and edges with styling to support a pipeline view in topology.

Implements UXDENG-138
Closes #7608

Additional issues:

WIP: needs updated styling

patternfly-build · 2022-06-24T12:48:14Z

Preview: https://patternfly-react-pr-7609.surge.sh

A11y report: https://patternfly-react-pr-7609-a11y.surge.sh

karthikjeeyar · 2022-06-27T16:17:09Z

packages/react-topology/src/pipelines/decorators/WhenDecorator.tsx

+        data-test="diamond-decorator"
+        className={css(
+          styles.topologyPipelinesWhenExpressionBackground,
+          status === WhenStatus.Met && styles.modifiers.success


Apart from Met and UnMet status, When Expression decorator in ODC has Inprogress and pending colors applied to it. Should we have that here as well ?
eg: status: InProgress

Ah, we should support those statuses as well then.

jeff-phillips-18 · 2022-06-28T17:34:29Z

styling should be all set, removing WIP status

jeff-phillips-18 · 2022-06-28T17:35:27Z

Updated surge: https://topology-pipeline-styles.surge.sh/

srambach

This looks really nice! Great job with all those css variables :-)
Left a comment about where I think you can redefine a color variable rather than setting it over. There might be other spots too?
Also, I don't see the hover state (thicker border) that is shown in the Marvel doc. Did the design change?
Finally, in dark theme there are a couple of things - I am guessing the blue icons should be the light blue? And on a solid (light) blue pill, I believe the text and icon should be dark.

srambach · 2022-06-29T20:24:22Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  --pf-topology-pipelines__pill__label__background--StrokeWidth: 1px;
+  --pf-topology-pipelines__pill__label--m-dragging--background--StrokeWidth: 1px;
+
+  --pf-topology-pipelines__pill--status--color: var(--pf-global--BorderColor--100);


I don't think this is used - but should be (see below)

srambach · 2022-06-29T20:27:03Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  fill: var(--pf-topology-pipelines__pill--m-selected--m-running__text-Color);
+}
+
+.pf-topology-pipelines__pill--status.pf-m-selected {


Here I think you want to add a rule for .pf-topology-pipelines__pill--status to have color: var(--pf-topology-pipelines__pill--status--color) and then in all the modifiers you can redefine that variable to be the correct status color variable.

jeff-phillips-18 · 2022-06-30T16:02:20Z

Thanks @srambach, I've updated

srambach

Some suggestions for keeping the naming consistent. Search and replace since I'm not going to catch them all ;-)

srambach · 2022-06-30T17:50:16Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  --pf-topology-pipelines__pill--m-disabled--Background--Fill: var(--pf-global--BackgroundColor--200);
+  --pf-topology-pipelines__pill--m-disabled--Background--Stroke: var(--pf-global--BorderColor--100);
+  --pf-topology-pipelines__pill--m-info--Background--Fill: var(--pf-global--primary-color--light-100);
+  --pf-topology-pipelines__pill--m-success--Background--Fill: var(--pf-global--success-color--100);
+  --pf-topology-pipelines__pill--m-warning--Background--Fill: var(--pf-global--warning-color--100);
+  --pf-topology-pipelines__pill--m-danger--Background--Fill: var(--pf-global--danger-color--100);
+  --pf-topology-pipelines__pill--m-selected--Background--Fill: var(--pf-global--active-color--100);


For consistency, I think it should be lower case and underscore __background in these.

srambach · 2022-06-30T17:51:05Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  --pf-topology-pipelines__pill--text-Color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--m-selected--text-Color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--m-selected--m-info--text-Color: var(--pf-global--Color--100);
+  --pf-topology-pipelines__pill--m-selected--m-danger--text-Color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--m-selected--m-warning--text-Color: var(--pf-global--Color--100);
+  --pf-topology-pipelines__pill--m-selected--m-running--text-Color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--m-selected--m-skipped--text-Color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--m-selected--m-pending--text-Color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--m-selected--m-canceled--text-Color: var(--pf-global--Color--light-100);


For these, I think you want __text-- so it's consistent with the other names.

srambach · 2022-06-30T17:55:47Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  --pf-topology-pipelines__pill--status--m-selected--color: var(--pf-global--active-color--100);
+  --pf-topology-pipelines__pill--status--m-info--color: var(--pf-global--primary-color--light-100);
+  --pf-topology-pipelines__pill--status--m-success--color: var(--pf-global--success-color--100);
+  --pf-topology-pipelines__pill--status--m-warning--color: var(--pf-global--warning-color--100);
+  --pf-topology-pipelines__pill--status--m-danger--color: var(--pf-global--danger-color--100);
+  --pf-topology-pipelines__pill--status--m-running--color: var(--pf-global--palette--blue-400);
+  --pf-topology-pipelines__pill--status--m-canceled--color: var(--pf-global--palette--black-600);
+  --pf-topology-pipelines__pill--status--m-skipped--color: var(--pf-global--palette--black-500);
+
+  --pf-topology-pipelines__pill--status--m-selected--m-info--color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--status--m-selected--m-success--color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--status--m-selected--m-warning--color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--status--m-selected--m-danger--color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--status--m-selected--m-running--color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--status--m-selected--m-canceled--color: var(--pf-global--Color--light-100);
+  --pf-topology-pipelines__pill--status--m-selected--m-skipped--color: var(--pf-global--Color--light-100);


And here, I think __pill__status or __pill-status. The first is more consistent with how topology currently is, and the second is more consistent with the rest of PatternFly.

srambach · 2022-06-30T18:05:17Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  --pf-topology-pipelines__pill__background--StrokeWidth: 2px;
+}
+
+.pf-topology-pipelines__pill--background {


.pf-topology-pipelines__pill__background here to match the class names

srambach · 2022-06-30T18:05:42Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  stroke: var(--pf-topology-pipelines__pill__background--Stroke);
+}
+
+.pf-topology-pipelines__pill--background.pf-m-disabled {


pf-topology-pipelines__pill__background to match the class names
... and the classes below, too.

srambach · 2022-06-30T18:07:45Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  --pf-topology-pipelines__pill--Color: var(--pf-topology-pipelines__pill--m-selected--m-running--text-Color);
+}
+
+.pf-topology-pipelines__pill--status {


Here, either .pf-topology-pipelines__pill-status or .pf-topology-pipelines__pill__status depending on what you change from the note above.

srambach · 2022-07-11T20:06:25Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+.pf-topology-pipelines__pill-text {
+  font-size: var(--pf-topology-pipelines__pill-text--FontSize);
+  font-family: var(--pf-topology-pipelines__pill-text--FontFamily);
+  fill: var(--pf-topology-pipelines__pill--Color);


I think this should actually be --pf-topology-pipelines__pill-text--Color and above line 42, you can initialize it to--pf-topology-pipelines__pill-text--Color: var(--pf-topology-pipelines__pill--Color). Then --pf-topology-pipelines__pill-text--Color can be reassigned in each of the modifiers below rather than setting fill directly.
Then line 96 for dark theme then may or may not be needed (I think it's probably good to keep), but currently I think it's not being referenced at all.
Caveat: I didn't try all this, it's just on inspection 🤪

@jeff-phillips-18 Do you want to reassign the variable rather than setting fill directly in lines 213-243? Otherwise, this PR LGTM. ⭐

srambach · 2022-07-11T20:13:56Z

packages/react-styles/src/css/components/Topology/topology-pipelines.css

+  stroke: var(--pf-topology-pipelines__when-expression--background--Stroke);
+  fill: var(--pf-topology-pipelines__when-expression--background--Fill);


I think you should be able to reassign this variable in the modifiers below (lines 344-358)
I'd probably rename the class and variables with when-expression--background and --edge to just use one - like when-expression-background and when-expression-edge since it's an element, not an attribute.

jeff-phillips-18 · 2022-07-12T13:36:29Z

@srambach Updated. Thanks!

tlabaj

LGTM

…an setting)

srambach

Great! 👍

patternfly-build · 2022-07-14T14:29:11Z

Your changes have been released in:

eslint-plugin-patternfly-react@4.64.0
@patternfly/react-catalog-view-extension@4.76.0
@patternfly/react-charts@6.78.0
@patternfly/react-code-editor@4.66.0
@patternfly/react-console@4.76.0
@patternfly/react-core@4.225.0
@patternfly/react-docs@5.86.0
@patternfly/react-icons@4.76.0
@patternfly/react-inline-edit-extension@4.70.0
demo-app-ts@4.185.0
@patternfly/react-integration@4.187.0
@patternfly/react-log-viewer@4.70.0
@patternfly/react-styles@4.75.0
@patternfly/react-table@4.94.0
@patternfly/react-tokens@4.77.0
@patternfly/react-topology@4.72.0
@patternfly/react-virtualized-extension@4.72.0
transformer-cjs-imports@4.63.0

Thanks for your contribution! 🎉

jeff-phillips-18 force-pushed the topology-pipelines branch 2 times, most recently from 983ca66 to 265acf2 Compare June 27, 2022 15:56

karthikjeeyar reviewed Jun 27, 2022

View reviewed changes

jeff-phillips-18 added 2 commits June 28, 2022 13:33

Add Topology pipeline support

d800f78

Update pipeline styles towards spec

d60db7e

jeff-phillips-18 force-pushed the topology-pipelines branch from 265acf2 to d60db7e Compare June 28, 2022 17:33

jeff-phillips-18 requested review from srambach, nicolethoen and tlabaj June 28, 2022 17:34

jeff-phillips-18 changed the title ~~WIP: feat(Topology): Add Topology pipeline support~~ feat(Topology): Add Topology pipeline support Jun 29, 2022

srambach reviewed Jun 29, 2022

View reviewed changes

update css vars rather than property directly

3b8fdf8

srambach reviewed Jun 30, 2022

View reviewed changes

jeff-phillips-18 added 2 commits June 30, 2022 16:53

Fixup styling vars and node border styles

fb139aa

Update class names to be more consistent

a4d8a22

srambach reviewed Jul 11, 2022

View reviewed changes

nicolethoen approved these changes Jul 12, 2022

View reviewed changes

Updates for when expression css vars

c2c0398

tlabaj approved these changes Jul 12, 2022

View reviewed changes

Update pill-text and action-icon color settings (update var rather th…

1bc057c

…an setting)

srambach approved these changes Jul 13, 2022

View reviewed changes

jeff-phillips-18 merged commit 4fb563f into patternfly:main Jul 14, 2022

jeff-phillips-18 deleted the topology-pipelines branch July 14, 2022 14:05

jenny-s51 pushed a commit to jenny-s51/patternfly-react that referenced this pull request Jul 26, 2022

feat(Topology): Add Topology pipeline support (patternfly#7609)

4f3e5c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Topology): Add Topology pipeline support #7609

feat(Topology): Add Topology pipeline support #7609

jeff-phillips-18 commented Jun 24, 2022

patternfly-build commented Jun 24, 2022 •

edited

Loading

karthikjeeyar Jun 27, 2022

jeff-phillips-18 Jun 28, 2022

jeff-phillips-18 commented Jun 28, 2022

jeff-phillips-18 commented Jun 28, 2022

srambach left a comment

srambach Jun 29, 2022

srambach Jun 29, 2022

jeff-phillips-18 commented Jun 30, 2022

srambach left a comment

srambach Jun 30, 2022

srambach Jun 30, 2022

srambach Jun 30, 2022

srambach Jun 30, 2022

srambach Jun 30, 2022

srambach Jun 30, 2022

srambach Jul 11, 2022

srambach Jul 12, 2022

srambach Jul 11, 2022

jeff-phillips-18 commented Jul 12, 2022

tlabaj left a comment

srambach left a comment

patternfly-build commented Jul 14, 2022

		stroke: var(--pf-topology-pipelines__when-expression--background--Stroke);
		fill: var(--pf-topology-pipelines__when-expression--background--Fill);

feat(Topology): Add Topology pipeline support #7609

feat(Topology): Add Topology pipeline support #7609

Conversation

jeff-phillips-18 commented Jun 24, 2022

patternfly-build commented Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Jun 28, 2022

jeff-phillips-18 commented Jun 28, 2022

srambach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Jun 30, 2022

srambach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Jul 12, 2022

tlabaj left a comment

Choose a reason for hiding this comment

srambach left a comment

Choose a reason for hiding this comment

patternfly-build commented Jul 14, 2022

patternfly-build commented Jun 24, 2022 •

edited

Loading