Removed edge customizations in favor of adding more CSS logic to styleEdge component #6874

leandroberetta · 2023-11-21T15:23:03Z

Resolves kiali/openshift-servicemesh-plugin#223

This PR aims to remove the customizations done to the edge component.

This PR uses the DefaultEdge from PF and it's modified by CSS in the StyleEdge component managed by Kiali (can be renamed to KialiEdge really).

frontend/src/pages/GraphPF/styles/styleEdge.tsx

jshaughn

This is promising, it seems like you may be able to eliminate the need for us to copy DefaultEdge. But there are some things that have changed or have been lost.

Edge width has changed. I'm actually OK thinning the edges a bit, but I don't think it's intentional. I think this is because if pathStyle is supplied we are not applying the entire style, only color. I think we should probably be honoring the entire style (note - the terminator needs only the color).
We lost the unhighlighting support
- Before:
- After:
We lost the add showTag prop and show scaled tag on hover (when showTag is false) customization.

leandroberetta · 2023-11-21T20:42:30Z

This is promising, it seems like you may be able to eliminate the need for us to copy DefaultEdge. But there are some things that have changed or have been lost.

Edge width has changed. I'm actually OK thinning the edges a bit, but I don't think it's intentional. I think this is because if pathStyle is supplied we are not applying the entire style, only color. I think we should probably be honoring the entire style (note - the terminator needs only the color).

We lost the unhighlighting support

Before:

After:

We lost the add showTag prop and show scaled tag on hover (when showTag is false) customization.

The edge width is intentional, I'm using 2 I guess now, I think it's cleaner.

The rest, yes, is missing, I forgot why this was a draft :). I will add it back.

…nhighlighted feature

Co-authored-by: Jay Shaughnessy <jshaughn@redhat.com>

jshaughn

The pathStyle issue is fixed, and I like the thinner edges. The unhighlighting is better but there is still a problem with unhighlighting the tags:

leandroberetta · 2023-11-22T17:31:44Z

The pathStyle issue is fixed, and I like the thinner edges. The unhighlighting is better but there is still a problem with unhighlighting the tags:

Good catch, now is fixed!

jshaughn

Edge tab unhighlighting is fixed, thanks! But, two more things. Hovering over an edge loses the edge coloring, and also does not trigger the unhighlighting:

leandroberetta · 2023-11-22T18:15:48Z

Edge tab unhighlighting is fixed, thanks! But, two more things. Hovering over an edge loses the edge coloring, and also does not trigger the unhighlighting:

Interesting, it seems to be a problem when hovering, this should be related to the override of css properties. Thanks.

ferhoyos · 2023-11-24T13:16:44Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx

+  let classes: string[] = [];
+
+  // Change edge color according to the pathStyle
+  const edgeClass = style({


Please use kialiStyle wrapper instead of style. It adds a prefix to CSS class name to fulfill Openshift plugin CSS requirement for OSSMC

ferhoyos · 2023-11-24T13:21:14Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx

@@ -31,7 +124,7 @@ const StyleEdgeComponent: React.FC<StyleEdgeProps> = ({ element, ...rest }) => {
    return newData;
  }, [data, detailsLevel]);

-  return <BaseEdge element={element} tagClass={tagClass} {...rest} {...passedData} />;
+  return <DefaultEdge className={classes?.join(' ')} element={element} {...rest} {...passedData} />;


Instead of join(' ') I would use classes method of typestyle library, a utility to join CSS classes conditionally

<DefaultEdge className={classes(...cssClasses)} element={element} {...rest} {...passedData} />;

leandroberetta · 2023-11-28T14:32:01Z

Edge tab unhighlighting is fixed, thanks! But, two more things. Hovering over an edge loses the edge coloring, and also does not trigger the unhighlighting:

Interesting, it seems to be a problem when hovering, this should be related to the override of css properties. Thanks.

@jshaughn I found the issue, there is a missing part due to the removal of the custom edge code, but sadly, I don't see yet how I can do it without touching the edge. At some point, in the edge, when hover is true, we are calling a Kiali function:

https://github.com/kiali/kiali/blob/master/frontend/src/pages/GraphPF/components/edge.tsx#L105

Probably we can think this a as contribution to PF's code, like adding a hover callback or something like this.

jshaughn · 2023-11-28T16:28:48Z

but sadly, I don't see yet how I can do it without touching the edge. At some point, in the edge, when hover is true, we are calling a Kiali function:

Right, this is what I meant in the file comment:

// add showTag prop and show scaled tag on hover (when showTag is false)

We need to decide what to do for this PR. We can:

Eliminate our custom Edge.tsx and regress with respect to this tag feature.
Re-introduce a more minimal Edge.tsx, that maintains the feature while we work with PFT for a supported alternative.

My preference would probably be option 2 because in addition to Edge.tsx we still have out customized versions for Node and Groups, so it's not the end of our customization. I think we should go through the same exercise for Nodes and Groups, seeing what we can move to the Style classes, and work with PFT for anything remaining.

leandroberetta · 2023-12-01T00:57:29Z

@jshaughn I finally could make the unhighlighting part:

I also fixed the edge style when hovering.

@ferhoyos you're feedback is addressed too. Thanks!.

ferhoyos · 2023-12-01T07:57:27Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx

+  // Change edge color according to the pathStyle
+  const edgeClass = kialiStyle({
+    $nest: {
+      '.pf-topology__edge__link': data.pathStyle


According to typestyle documentation (https://typestyle.github.io/#/core), it is best practice to add symbol $ to overwrite third party nested CSS classes:

const container = style({ $nest: { '& .third-party-class': { color: 'red' } } });

Most of the times it works fine without the & symbol, but not in all cases (if the third party CSS class has more specifity than yours). More information here: #6562

ferhoyos · 2023-12-04T16:33:36Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx

+    $nest: {
+      '& .pf-topology__edge.pf-m-hover': {
+        $nest: {
+          '& .pf-topology__edge__link, .pf-topology-connector-arrow': data.pathStyle


Each selector is independent, so you have to add '&' for each selector:
'& .pf-topology__edge__link, & .pf-topology-connector-arrow': data.pathStyle

ferhoyos

LGTM from the CSS point of view, you have learnt a lot in the process and done a great job @leandroberetta ;-). Anyway I would wait for @jshaughn review from topology features point of view.

jshaughn

I think all of the prior comments are resolved! And we have decided to temporarily remove the tag hide/hover customization and contribute that feature to PFT itself. So that feature is expected to not be there.

There is a regression in showing lock icons on edge labels, this can be fixed by accepting the recommended changes in my comments.

Also, I'm seeing maybe an issue in trace overlay, but I don't think it has to do with this PR, it may be nothing, but it seems sometimes the overlay stops before the selected node. Anyway, not a blocker for this.

I think if you fix the lock icon issue we can approve.

jshaughn · 2023-12-05T18:44:20Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx


-// This is the registered Edge component override that utilizes our customized Edge.tsx component.
+// This is our styled edge component registered in stylesComponentFactory.tsx.  It is responsible for adding our custom customizations that get passed down to DefaultEdge.  The current customizations:


Suggested change

// This is our styled edge component registered in stylesComponentFactory.tsx. It is responsible for adding our custom customizations that get passed down to DefaultEdge. The current customizations:

// This is our styled edge component registered in stylesComponentFactory.tsx. It is responsible for adding customizations that then get passed down to DefaultEdge. The current customizations:

frontend/src/pages/GraphPF/styles/styleEdge.tsx

jshaughn · 2023-12-05T19:44:24Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx

-const tagClass = kialiStyle({
-  fontFamily: 'Verdana,Arial,Helvetica,sans-serif,pficon'
-});


This is needed to support the lock icons on edge tags.

jshaughn · 2023-12-05T19:45:05Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx

-  return <BaseEdge element={element} tagClass={tagClass} {...rest} {...passedData} />;
+  return (
+    <g style={{ opacity: opacity }} onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave}>
+      <DefaultEdge className={classes(...cssClasses)} element={element} {...rest} {...passedData} />


Suggested change

<DefaultEdge className={classes(...cssClasses)} element={element} {...rest} {...passedData} />

<DefaultEdge className={classes(...cssClasses)} element={element} tagClass={tagClass} {...rest} {...passedData} />

This is needed to support the lock icons on edge tags.

leandroberetta · 2023-12-05T20:14:53Z

I think all of the prior comments are resolved! And we have decided to temporarily remove the tag hide/hover customization and contribute that feature to PFT itself. So that feature is expected to not be there.

There is a regression in showing lock icons on edge labels, this can be fixed by accepting the recommended changes in my comments.

Also, I'm seeing maybe an issue in trace overlay, but I don't think it has to do with this PR, it may be nothing, but it seems sometimes the overlay stops before the selected node. Anyway, not a blocker for this.

I think if you fix the lock icon issue we can approve.

Thanks for your review @jshaughn.

I vote for remove for now the hide/hover feature (there is a PR already for that in PF repo: patternfly/react-topology#123, this issue is mentioned in the epic: kiali/openshift-servicemesh-plugin#99).

I will address the feedback.

jshaughn

LGTM (one non-blocking comment)

jshaughn · 2023-12-06T14:24:49Z

frontend/src/pages/GraphPF/styles/styleEdge.tsx

+//   data.isFind?: boolean                // adds graph-find overlay
+//   data.isUnhighlighted?: boolean       // adds unhighlight effects
+//   data.hasSpans?: Span[]               // adds trace overlay
+//   add showTag prop and show scaled tag on hover (when showTag is false)


not a blocker - please remove this line in a future PR, this is not done here, we are deferring to our PFT contribution for this, right?

removing edge customizations

81fc5ff

leandroberetta requested a review from jshaughn November 21, 2023 15:30

leandroberetta self-assigned this Nov 21, 2023

leandroberetta marked this pull request as ready for review November 21, 2023 17:09

jshaughn reviewed Nov 21, 2023

View reviewed changes

frontend/src/pages/GraphPF/styles/styleEdge.tsx Outdated Show resolved Hide resolved

jshaughn reviewed Nov 21, 2023

View reviewed changes

leandroberetta and others added 3 commits November 21, 2023 18:37

removing seting the stroke width in the styleedge, also, adding the u…

3b98812

…nhighlighted feature

Update frontend/src/pages/GraphPF/styles/styleEdge.tsx

34dff43

Co-authored-by: Jay Shaughnessy <jshaughn@redhat.com>

fixing is unhighlighted style

13d774d

leandroberetta requested a review from jshaughn November 22, 2023 12:03

jshaughn requested changes Nov 22, 2023

View reviewed changes

hide the edge tag too when unhighlighting

8ea6ca0

jshaughn requested changes Nov 22, 2023

View reviewed changes

ferhoyos requested changes Nov 24, 2023

View reviewed changes

fixing issue when unhighlighting and fix hovering styles

35e4eba

ferhoyos requested changes Dec 1, 2023

View reviewed changes

improving css specificity

c968447

leandroberetta requested review from ferhoyos and jshaughn December 1, 2023 17:02

ferhoyos reviewed Dec 4, 2023

View reviewed changes

adding missing &

3f2ef83

leandroberetta requested a review from ferhoyos December 5, 2023 11:47

ferhoyos previously approved these changes Dec 5, 2023

View reviewed changes

jshaughn requested changes Dec 5, 2023

View reviewed changes

lock icon on edges

1fbad9e

leandroberetta dismissed ferhoyos’s stale review via 1fbad9e December 6, 2023 12:43

leandroberetta requested a review from jshaughn December 6, 2023 13:33

jshaughn approved these changes Dec 6, 2023

View reviewed changes

leandroberetta merged commit 2ed984f into kiali:master Dec 6, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed edge customizations in favor of adding more CSS logic to styleEdge component #6874

Removed edge customizations in favor of adding more CSS logic to styleEdge component #6874

leandroberetta commented Nov 21, 2023 •

edited

Loading

jshaughn left a comment

leandroberetta commented Nov 21, 2023

jshaughn left a comment

leandroberetta commented Nov 22, 2023

jshaughn left a comment

leandroberetta commented Nov 22, 2023

ferhoyos Nov 24, 2023

ferhoyos Nov 24, 2023

leandroberetta commented Nov 28, 2023 •

edited

Loading

jshaughn commented Nov 28, 2023

leandroberetta commented Dec 1, 2023

ferhoyos Dec 1, 2023

ferhoyos Dec 4, 2023

ferhoyos left a comment

jshaughn left a comment

jshaughn Dec 5, 2023

jshaughn Dec 5, 2023

jshaughn Dec 5, 2023

leandroberetta commented Dec 5, 2023

jshaughn left a comment

jshaughn Dec 6, 2023


		// This is the registered Edge component override that utilizes our customized Edge.tsx component.
		// This is our styled edge component registered in stylesComponentFactory.tsx. It is responsible for adding our custom customizations that get passed down to DefaultEdge. The current customizations:

	<DefaultEdge className={classes(...cssClasses)} element={element} {...rest} {...passedData} />
	<DefaultEdge className={classes(...cssClasses)} element={element} tagClass={tagClass} {...rest} {...passedData} />

Removed edge customizations in favor of adding more CSS logic to styleEdge component #6874

Removed edge customizations in favor of adding more CSS logic to styleEdge component #6874

Conversation

leandroberetta commented Nov 21, 2023 • edited Loading

jshaughn left a comment

Choose a reason for hiding this comment

leandroberetta commented Nov 21, 2023

jshaughn left a comment

Choose a reason for hiding this comment

leandroberetta commented Nov 22, 2023

jshaughn left a comment

Choose a reason for hiding this comment

leandroberetta commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandroberetta commented Nov 28, 2023 • edited Loading

jshaughn commented Nov 28, 2023

leandroberetta commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferhoyos left a comment

Choose a reason for hiding this comment

jshaughn 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

leandroberetta commented Dec 5, 2023

jshaughn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandroberetta commented Nov 21, 2023 •

edited

Loading

leandroberetta commented Nov 28, 2023 •

edited

Loading