feat(Topology): Add Topology pipeline support by jeff-phillips-18 · Pull Request #7609 · patternfly/patternfly-react

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

+        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

+  --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

+  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

+  --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

+  --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

+  --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

+  --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

+  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

+  --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

+.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

+  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 nicolethoen, srambach 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

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

Uh oh!

Conversation

jeff-phillips-18 commented Jun 24, 2022

Uh oh!

patternfly-build commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jun 28, 2022

Uh oh!

jeff-phillips-18 commented Jun 28, 2022

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jun 30, 2022

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeff-phillips-18 commented Jul 12, 2022

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

srambach left a comment

Choose a reason for hiding this comment

Uh oh!

patternfly-build commented Jul 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

patternfly-build commented Jun 24, 2022 •

edited

Loading