Bug 1797149: fix inconsistencies in topology node styling #4117

christianvogt · 2020-01-30T02:56:03Z

Fixes:
Fixes https://issues.redhat.com/browse/ODC-2903

Analysis / Root cause:
There is a lot of similar code across the various react components representing the nodes and groups in topology. Unfortunately this is error prone as we've added new states and new react components and some of them simply aren't done in a consistent manner.

Solution Description:
Went through each topology node and group and updated the styles for consistent naming and css rule specificity.

Requires a future run through with @openshift/team-devconsole-ux to ensure colors are all correct. At least now the styles are consistent across nodes.

Screen shots / Gifs for design review:
selected:

find:

Browser conformance:

Chrome
Firefox
Safari
Edge

jeff-phillips-18 · 2020-01-30T15:56:04Z

frontend/packages/dev-console/src/components/topology/components/nodes/KnativeService.tsx

@@ -10,7 +8,7 @@ import {
 } from '@console/topology';
 import { modelFor, referenceFor } from '@console/internal/module/k8s';
 import { useAccessReview } from '@console/internal/components/utils';
-import { getTopologyFilters, TopologyFilters } from '../../filters/filter-utils';
+import { TopologyFilters } from '../../filters/filter-utils';


This can be removed along with the optional filters prop

openshift-ci-robot · 2020-02-01T02:59:53Z

@christianvogt: This pull request references Bugzilla bug 1797149, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1797149: fix inconsistencies in topology node styling

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-02-01T05:09:57Z

@christianvogt: This pull request references Bugzilla bug 1797149, which is valid.

In response to this:

Bug 1797149: fix inconsistencies in topology node styling

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-02-01T05:12:28Z

@christianvogt: This pull request references Bugzilla bug 1797149, which is valid.

In response to this:

Bug 1797149: fix inconsistencies in topology node styling

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jeff-phillips-18 · 2020-02-03T12:37:46Z

frontend/packages/dev-console/src/components/topology/components/nodes/KnativeService.scss

+  &__bg {
+    fill: #d1d1d1;
+    fill-opacity: 0.5;
+    stroke: #bbbbbb;


We use #d1d1d1 and #bbbbb in the three groups' styles. Could we add these as variables in the topology-utils.scss ?

jeff-phillips-18 · 2020-02-03T12:40:25Z

frontend/packages/dev-console/src/components/topology/components/nodes/Application.scss

-      cursor: grab;
-    }
-
+    fill: var(--pf-global--Color--300);
    & text {
      fill: #fff;


while we're in here

Suggested change

fill: #fff;

fill: var(--pf-global--Color--light-100);

jeff-phillips-18 · 2020-02-03T12:47:42Z

frontend/packages/dev-console/src/components/topology/components/groups/HelmReleaseGroup.tsx

-              'is-filtered': filtered,
-              'is-dragging': dragging,
-            })}
+            className="odc-base-node__label"
            x={x + width / 2}
            y={y + height + 20}


Other groupings use 30 here. Maybe a constant or default value in SvgBoxedText? (paddingX, paddingY, and truncate as well?)

Let's improve this as part of a PR to to fix the label padding.

serenamarie125

@christianvogt did you end up doing the 24px between components/groupings and their label?

christianvogt · 2020-02-03T17:26:02Z

@serenamarie125 that's not part of this PR. I focused on node state styles. I think we're better off doing a followup PR for any further styling issues as this PR is already large enough.

jeff-phillips-18 · 2020-02-03T19:11:57Z

/retest

jeff-phillips-18 · 2020-02-03T19:27:21Z

frontend/packages/dev-console/src/components/topology/components/edges/EventSourceLink.scss

@@ -1,8 +1,7 @@
 .odc-base-edge.odc-event-source-link {
  --edge--stroke: #bbbbbb;


Maybe $edge-stroke-color

jeff-phillips-18 · 2020-02-03T19:27:44Z

frontend/packages/dev-console/src/components/topology/components/edges/TrafficLink.scss

@@ -1,8 +1,6 @@
 .odc-base-edge.odc-traffic-link {
  --edge--stroke-width: 0;
  --edge--stroke: #bbbbbb;


christianvogt · 2020-02-03T20:57:21Z

@jeff-phillips-18 added a new variable default-edge-stroke-color and adjusted the css rules around edge. Also consolidated the rules around edge __link as discussed on slack.

jeff-phillips-18 · 2020-02-03T21:13:58Z

/lgtm

openshift-ci-robot · 2020-02-03T21:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jeff-phillips-18

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/packages/dev-console/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jeff-phillips-18 · 2020-02-03T22:38:45Z

/retest

openshift-ci-robot · 2020-02-04T02:20:15Z

@christianvogt: All pull requests linked via external trackers have merged. Bugzilla bug 1797149 has been moved to the MODIFIED state.

In response to this:

Bug 1797149: fix inconsistencies in topology node styling

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/dev-console Related to dev-console labels Jan 30, 2020

openshift-ci-robot requested review from abhi-kn and karthikjeeyar January 30, 2020 02:56

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2020

christianvogt force-pushed the filter-highlight branch from b9f4f59 to 1509a00 Compare January 30, 2020 03:02

jeff-phillips-18 reviewed Jan 30, 2020

View reviewed changes

christianvogt force-pushed the filter-highlight branch from 1509a00 to 3bdd717 Compare February 1, 2020 02:40

christianvogt force-pushed the filter-highlight branch from 3bdd717 to 8d2aa1b Compare February 1, 2020 02:43

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2020

christianvogt force-pushed the filter-highlight branch from 8d2aa1b to e9127c5 Compare February 1, 2020 02:45

christianvogt changed the title ~~[WIP] fix inconsistencies in topology node styling~~ Bug 1797149: fix inconsistencies in topology node styling Feb 1, 2020

openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 1, 2020

christianvogt force-pushed the filter-highlight branch from e9127c5 to 5fce990 Compare February 1, 2020 03:01

jeff-phillips-18 reviewed Feb 3, 2020

View reviewed changes

serenamarie125 reviewed Feb 3, 2020

View reviewed changes

christianvogt force-pushed the filter-highlight branch from 5fce990 to c842718 Compare February 3, 2020 19:15

jeff-phillips-18 reviewed Feb 3, 2020

View reviewed changes

christianvogt added 3 commits February 3, 2020 15:56

fix consistency issues in node and edge styling

0bcea9e

add unit tests fo useSearchFilter

91305ce

update scss variables

ce23d77

christianvogt force-pushed the filter-highlight branch from c842718 to ce23d77 Compare February 3, 2020 20:56

jeff-phillips-18 approved these changes Feb 3, 2020

View reviewed changes

openshift-ci-robot assigned jeff-phillips-18 Feb 3, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2020

openshift-merge-robot merged commit 4359aad into openshift:master Feb 4, 2020

spadgett added this to the v4.4 milestone Feb 4, 2020

christianvogt deleted the filter-highlight branch August 20, 2020 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1797149: fix inconsistencies in topology node styling #4117

Bug 1797149: fix inconsistencies in topology node styling #4117

christianvogt commented Jan 30, 2020 •

edited

jeff-phillips-18 Jan 30, 2020

openshift-ci-robot commented Feb 1, 2020

openshift-ci-robot commented Feb 1, 2020

openshift-ci-robot commented Feb 1, 2020

jeff-phillips-18 Feb 3, 2020

jeff-phillips-18 Feb 3, 2020

jeff-phillips-18 Feb 3, 2020

christianvogt Feb 3, 2020

serenamarie125 left a comment

christianvogt commented Feb 3, 2020

jeff-phillips-18 commented Feb 3, 2020

jeff-phillips-18 Feb 3, 2020

jeff-phillips-18 Feb 3, 2020

christianvogt commented Feb 3, 2020

jeff-phillips-18 commented Feb 3, 2020

openshift-ci-robot commented Feb 3, 2020

jeff-phillips-18 commented Feb 3, 2020

openshift-ci-robot commented Feb 4, 2020

		@@ -1,8 +1,7 @@
		.odc-base-edge.odc-event-source-link {
		--edge--stroke: #bbbbbb;

Bug 1797149: fix inconsistencies in topology node styling #4117

Bug 1797149: fix inconsistencies in topology node styling #4117

Conversation

christianvogt commented Jan 30, 2020 • edited

jeff-phillips-18 Jan 30, 2020

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 1, 2020

openshift-ci-robot commented Feb 1, 2020

openshift-ci-robot commented Feb 1, 2020

jeff-phillips-18 Feb 3, 2020

Choose a reason for hiding this comment

jeff-phillips-18 Feb 3, 2020

Choose a reason for hiding this comment

jeff-phillips-18 Feb 3, 2020

Choose a reason for hiding this comment

christianvogt Feb 3, 2020

Choose a reason for hiding this comment

serenamarie125 left a comment

Choose a reason for hiding this comment

christianvogt commented Feb 3, 2020

jeff-phillips-18 commented Feb 3, 2020

jeff-phillips-18 Feb 3, 2020

Choose a reason for hiding this comment

jeff-phillips-18 Feb 3, 2020

Choose a reason for hiding this comment

christianvogt commented Feb 3, 2020

jeff-phillips-18 commented Feb 3, 2020

openshift-ci-robot commented Feb 3, 2020

jeff-phillips-18 commented Feb 3, 2020

openshift-ci-robot commented Feb 4, 2020

christianvogt commented Jan 30, 2020 •

edited