Bug 1880410: convert pipeline visualization node to SVG #6676

rottencandy · 2020-09-18T13:09:12Z

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

Analysis / Root cause:
There are several rendering issues with the Pipeline Visualization Nodes (aka Tasks). This needs to be handled better and the whole thing is currently rendered through a foriegnObject inside the SVG visualization.

Solution Description:
Convert pipeline visualization node to SVG.

Screen shots / Gifs for design review:
(Should ideally be unchanged)

Unit test coverage report:
Unchanged.

Browser conformance:

Chrome
Firefox
Safari
Edge

openshift-ci-robot · 2020-09-18T13:09:19Z

@rottencandy: This pull request references Bugzilla bug 1880410, 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.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

[WIP]Bug 1880410: convert pipeline visualization node to SVG

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-09-18T14:18:08Z

@rottencandy: This pull request references Bugzilla bug 1880410, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

[WIP]Bug 1880410: convert pipeline visualization node to SVG

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.

rottencandy · 2020-09-18T14:18:51Z

/hold
Will be worked on in the next sprint.

rottencandy · 2020-10-05T12:44:55Z

/bugzilla refresh

openshift-ci-robot · 2020-10-05T12:44:59Z

@rottencandy: This pull request references Bugzilla bug 1880410, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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-10-07T16:47:05Z

@rottencandy: This pull request references Bugzilla bug 1880410, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1880410: convert pipeline visualization node to SVG

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.

rottencandy · 2020-10-07T16:47:12Z

/hold cancel

rottencandy · 2020-10-07T17:23:41Z

/kind bug

rottencandy · 2020-10-08T06:33:23Z

/retest

jerolimov · 2020-10-09T19:02:31Z

Code change looks valid, but I find some UI difference.

In view mode:
Before:

Now:

The vertical alignment is not centered anymore, can you center it by increasing the (line) height or the y-offset here?
The old version uses the complete width of the boxes for longer task names (first box, I repeated the task name multiple times).
There is a little bit "more shadow" in the old version which is shown in the new one only on hover. I like it! I would prefer if we accept the new version.

In edit mode:
Before:

Now:

I think the vertical alignment could also be one pixel (sorry :P) lower.
I don't know why, but the "really long" task name overlaps the box. Only in the edit case when it does not find the task reference!
The bottom border is now bigger than in the old version also when the box is not selected. Not bad, but different.

rottencandy · 2020-10-12T19:06:58Z

Thanks for the review @jerolimov !

The vertical alignment is not centered anymore, can you center it by increasing the (line) height or the y-offset here?

Updated.

The old version uses the complete width of the boxes for longer task names (first box, I repeated the task name multiple times).

Updated the max length. I think there will be some differences here. The old version used a CSS solution(using text-overflow considers font width) but the new one uses a JS solution(does not consider font width), there seems to be no way to truncate text in SVG.

There is a little bit "more shadow" in the old version which is shown in the new one only on hover. I like it! I would prefer if we accept the new version.

I tried to replicate the old version which also had hover shadow, albeit was very subtle. I can add the non-hover shadow as well if you think it's better.

In edit mode:

I think the vertical alignment could also be one pixel (sorry :P) lower.

Updated.

I don't know why, but the "really long" task name overlaps the box. Only in the edit case when it does not find the task reference!

Thank you for catching this, I seem to have missed it. Updated.

The bottom border is now bigger than in the old version also when the box is not selected. Not bad, but different.

Not sure why it appears bigger, I've set it to the smallest possible width of 1px(under __trigger-underline in TaskListNode.scss). I'll go through SVG attrs again to see if I find something.

EDIT:
I found out that it doesn't need to be in px, and can also have decimal values. Updated:

rottencandy · 2020-10-12T19:11:27Z

Also about the bottom border, I think there is a bit of inconsistency between browsers.
Didn't notice this earlier since I was testing mostly on Firefox, the button appears slightly different on Chromium.

Firefox vs. Chromium:

Should I follow the Firefox appearance, or the Chromium one?
cc: @andrewballantyne

andrewballantyne

Having some hovering issues.

When I hover over an item and add a parallel item, and then mouse up over the node and slowly to the dropdown, it removes and reappears the "+" icons on the above node.

andrewballantyne · 2021-01-25T22:22:43Z

...s/pipelines-plugin/src/components/pipelines/detail-page-tabs/pipeline-details/StatusIcon.tsx

+      return (
+        <SyncAltIcon
+          {...props}
+          className={cx('fa-spin', { 'pipeline-status-icon__spin': shiftOrigin })}


Suggested change

className={cx('fa-spin', { 'pipeline-status-icon__spin': shiftOrigin })}

className={cx('fa-spin', { 'pipeline-status-icon--spin': shiftOrigin })}

BEM is

Block (pipeline-status-icon)

Element (pipeline-status-icon__someWrapper) -- kinda like a sub Block

Modifier (pipeline-status-icon--spin) -- when you want it to be "modified" based on a prop

andrewballantyne · 2021-01-25T22:35:02Z

Ah, it's the tooltip capturing the mouse-over and I think it persists at that point.

andrewballantyne · 2021-01-26T12:39:46Z

/hold cancel

andrewballantyne · 2021-01-26T12:40:12Z

FWIW, frontend test failure is lack of yarn i18n

rottencandy · 2021-01-27T11:31:24Z

Ah, it's the tooltip capturing the mouse-over and I think it persists at that point.

Replacing the onMouse handlers with useHover seems to fix it.

andrewballantyne

Tiny nit, will test it out once I finish compiling 🙂

andrewballantyne · 2021-01-27T14:51:01Z

frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-topology/TaskListNode.scss

+  }
+  &__trigger-underline {
+    fill: var(--pf-global--Color--100);
+    &__hover {


Suggested change

&__hover {

&--hover {

andrewballantyne · 2021-01-27T15:05:09Z

/cc @karthikjeeyar @jerolimov

Please one of you toss a review on this as well. If I don't see any review by EOD tomorrow, I'll LGTM it.

jerolimov · 2021-01-27T17:48:16Z

/retest

karthikjeeyar · 2021-01-28T16:25:29Z

Verified locally, works fine. Thanks @rottencandy
/lgtm

openshift-ci-robot · 2021-01-28T16:25:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, jerolimov, karthikjeeyar, rottencandy

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/pipelines-plugin/OWNERS~~ [andrewballantyne]

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

openshift-ci · 2021-01-28T19:57:18Z

@rottencandy: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/kubevirt-plugin	`0c94891`	link	`/test kubevirt-plugin`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-bot · 2021-01-28T21:19:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-01-28T22:51:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2021-01-29T02:08:53Z

@rottencandy: All pull requests linked via external trackers have merged:

openshift/console#6676

Bugzilla bug 1880410 has been moved to the MODIFIED state.

In response to this:

Bug 1880410: convert pipeline visualization node to SVG

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 the component/dev-console Related to dev-console label Sep 18, 2020

openshift-ci-robot requested review from divyanshiGupta and jeff-phillips-18 September 18, 2020 13:09

rottencandy changed the title ~~[WIP]Bug 1880410: convert pipeline visualization node to SVG~~ Bug 1880410: convert pipeline visualization node to SVG Sep 18, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2020

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2020

rottencandy force-pushed the svgtask branch from 2b8eb90 to 5bca94e Compare October 5, 2020 12:44

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2020

rottencandy force-pushed the svgtask branch from 5bca94e to a523fbf Compare October 7, 2020 16:44

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2020

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 7, 2020

rottencandy force-pushed the svgtask branch from a523fbf to d9b3f2f Compare October 12, 2020 18:48

rottencandy force-pushed the svgtask branch 2 times, most recently from 1748565 to 61c05ae Compare October 13, 2020 15:52

rottencandy changed the title ~~[WIP] Bug 1880410: convert pipeline visualization node to SVG~~ Bug 1880410: convert pipeline visualization node to SVG Jan 19, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2021

andrewballantyne suggested changes Jan 25, 2021

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 26, 2021

rottencandy force-pushed the svgtask branch from 6951cbd to fde7f3b Compare January 27, 2021 11:29

openshift-ci-robot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Jan 27, 2021

andrewballantyne suggested changes Jan 27, 2021

View reviewed changes

andrewballantyne approved these changes Jan 27, 2021

View reviewed changes

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

convert pipeline visualization node to SVG

1854b42

rottencandy force-pushed the svgtask branch from fde7f3b to 1854b42 Compare January 27, 2021 15:03

openshift-ci-robot requested review from jerolimov and karthikjeeyar January 27, 2021 15:06

openshift-ci-robot assigned karthikjeeyar Jan 28, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2021

openshift-merge-robot merged commit aa58c29 into openshift:master Jan 29, 2021

spadgett added this to the v4.7 milestone Feb 1, 2021

rottencandy deleted the svgtask branch February 8, 2021 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1880410: convert pipeline visualization node to SVG #6676

Bug 1880410: convert pipeline visualization node to SVG #6676

rottencandy commented Sep 18, 2020 •

edited

openshift-ci-robot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

rottencandy commented Sep 18, 2020

rottencandy commented Oct 5, 2020

openshift-ci-robot commented Oct 5, 2020

openshift-ci-robot commented Oct 7, 2020

rottencandy commented Oct 7, 2020

rottencandy commented Oct 7, 2020

rottencandy commented Oct 8, 2020

jerolimov commented Oct 9, 2020

rottencandy commented Oct 12, 2020 •

edited

rottencandy commented Oct 12, 2020

andrewballantyne left a comment

andrewballantyne Jan 25, 2021

rottencandy Jan 27, 2021

andrewballantyne commented Jan 25, 2021 •

edited

andrewballantyne commented Jan 26, 2021

andrewballantyne commented Jan 26, 2021

rottencandy commented Jan 27, 2021

andrewballantyne left a comment

andrewballantyne Jan 27, 2021

rottencandy Jan 27, 2021

andrewballantyne commented Jan 27, 2021

jerolimov commented Jan 27, 2021

karthikjeeyar commented Jan 28, 2021

openshift-ci-robot commented Jan 28, 2021

openshift-ci bot commented Jan 28, 2021 •

edited

openshift-bot commented Jan 28, 2021

openshift-bot commented Jan 28, 2021

openshift-ci-robot commented Jan 29, 2021

	className={cx('fa-spin', { 'pipeline-status-icon__spin': shiftOrigin })}
	className={cx('fa-spin', { 'pipeline-status-icon--spin': shiftOrigin })}

Bug 1880410: convert pipeline visualization node to SVG #6676

Bug 1880410: convert pipeline visualization node to SVG #6676

Conversation

rottencandy commented Sep 18, 2020 • edited

openshift-ci-robot commented Sep 18, 2020

openshift-ci-robot commented Sep 18, 2020

rottencandy commented Sep 18, 2020

rottencandy commented Oct 5, 2020

openshift-ci-robot commented Oct 5, 2020

openshift-ci-robot commented Oct 7, 2020

rottencandy commented Oct 7, 2020

rottencandy commented Oct 7, 2020

rottencandy commented Oct 8, 2020

jerolimov commented Oct 9, 2020

rottencandy commented Oct 12, 2020 • edited

rottencandy commented Oct 12, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Jan 25, 2021

Choose a reason for hiding this comment

rottencandy Jan 27, 2021

Choose a reason for hiding this comment

andrewballantyne commented Jan 25, 2021 • edited

andrewballantyne commented Jan 26, 2021

andrewballantyne commented Jan 26, 2021

rottencandy commented Jan 27, 2021

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Jan 27, 2021

Choose a reason for hiding this comment

rottencandy Jan 27, 2021

Choose a reason for hiding this comment

andrewballantyne commented Jan 27, 2021

jerolimov commented Jan 27, 2021

karthikjeeyar commented Jan 28, 2021

openshift-ci-robot commented Jan 28, 2021

openshift-ci bot commented Jan 28, 2021 • edited

openshift-bot commented Jan 28, 2021

openshift-bot commented Jan 28, 2021

openshift-ci-robot commented Jan 29, 2021

rottencandy commented Sep 18, 2020 •

edited

rottencandy commented Oct 12, 2020 •

edited

andrewballantyne commented Jan 25, 2021 •

edited

openshift-ci bot commented Jan 28, 2021 •

edited