Bug 1923721: Handle pipeline svg icon spin using a wrapped <g> element #8007

rottencandy · 2021-02-01T17:44:19Z

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

Analysis / Root cause:
Chromium does not animate nested SVGs.

Solution Description:
Wrap the SVG in a <g> element and animate that instead.

Screen shots / Gifs for design review:

tmp.mp4

Browser conformance:

Chrome
Firefox
Safari
Edge

/kind bug

openshift-ci-robot · 2021-02-01T17:44:25Z

@rottencandy: This pull request references Bugzilla bug 1923721, 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.7.0) matches configured target release for branch (4.7.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1923721: Handle pipeline svg icon spin using a wrapped element

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 · 2021-02-01T17:44:46Z

/cc @jerolimov @karthikjeeyar

karthikjeeyar · 2021-02-02T06:07:02Z

Thanks @rottencandy for this fix. works fine now
/lgtm

jerolimov · 2021-02-02T11:02:45Z

...in/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.scss

+  &--icon-spin {
+    transform-origin: 3.5% 14%;
+  }


Sorry for the late reply. I was yesterday already confused by this "magic numbers" here. It looks like this are the % width and height of the parent container of the icon.

You can set this also to the width and height (-0.8, don't know why 😕 ) if you want.

I tested this in the dev tools with this changes and it looks good for me.

PipelineVisualizationTask.scss:

Suggested change

&--icon-spin {

transform-origin: 3.5% 14%;

}

&--icon-spin {

transform-origin: 16.61px 16.61px;

}

When I also changed the viewBox in PipelineVisualizationTask.tsx to viewBox="-5 -5 20 21" to vertical align the Icon a little bit, I need to change this to this values:

Suggested change

&--icon-spin {

transform-origin: 3.5% 14%;

}

&--icon-spin {

transform-origin: 16.61px 15.61px;

}

Wdyt?

I tested this because I'm interested into this magic number. If Andrew gives an approval here I'm also fine with that.

Updated transform-origin to 16.61px. I think values in px would be better than percentages. It's confusing, I thought that it would be calculated for the element that gets the animation, and not it's parent.
And fa-spin seems to animate simply using rotate.

jerolimov · 2021-02-02T11:57:46Z

/assign @andrewballantyne

nemesis09 · 2021-02-03T19:51:17Z

verified the changes locally
works as expected.
/lgtm

andrewballantyne · 2021-02-04T14:53:23Z

...gin/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.tsx

+          <svg
+            width={30}
+            height={30}
+            viewBox="-5 -5 20 20"


Suggested change

viewBox="-5 -5 20 20"

viewBox="-5 -4 20 20"

The icon sits a bit too low, let's bring it more to the center of the node.

eg:

-5 y value:

-4 y value:

Slight difference but more clean imo.

Updated. Had to also change transform-origin to re center the icon spin.

transform-origin could be more into the direction of 16.7px 15.1px . But don't want block the PR.

Still a confusing solution we have here. 😕

andrewballantyne

/lgtm

jerolimov · 2021-02-04T17:06:49Z

/lgtm

jerolimov

Looks fine. Thanks for the extra rounds 😄

/lgtm

openshift-ci-robot · 2021-02-04T17:07:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, jerolimov, karthikjeeyar, nemesis09, 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

christianvogt · 2021-02-04T21:43:17Z

@rottencandy please create an issue on PF to add support for embedding react icons inside an existing SVG without PF nesting a new <svg> element. For whatever reason, adding the css animation to the nested svg has no affect. It would be good if we can use PF icons and the animation without having to create this wrapper.

openshift-bot · 2021-02-04T21:52:46Z

/retest

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

openshift-ci-robot · 2021-02-05T01:34:52Z

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

openshift/console#8007

Bugzilla bug 1923721 has been moved to the MODIFIED state.

In response to this:

Bug 1923721: Handle pipeline svg icon spin using a wrapped element

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 · 2021-02-05T03:10:30Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-console	`6f4861b`	link	`/test e2e-gcp-console`

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-ci-robot requested review from jerolimov, karthikjeeyar and christianvogt February 1, 2021 17:44

openshift-ci-robot assigned karthikjeeyar Feb 2, 2021

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

jerolimov reviewed Feb 2, 2021

View reviewed changes

openshift-ci-robot assigned andrewballantyne Feb 2, 2021

rottencandy force-pushed the pipeline-icon-spin branch from cfb85ca to 6c77af2 Compare February 2, 2021 17:24

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2021

openshift-ci-robot assigned nemesis09 Feb 3, 2021

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

andrewballantyne reviewed Feb 4, 2021

View reviewed changes

rottencandy force-pushed the pipeline-icon-spin branch from 6c77af2 to 745db1f Compare February 4, 2021 15:09

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2021

andrewballantyne approved these changes Feb 4, 2021

View reviewed changes

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 4, 2021

handle pipeline svg spin using wrapped <g>

6f4861b

rottencandy force-pushed the pipeline-icon-spin branch from 745db1f to 6f4861b Compare February 4, 2021 17:02

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2021

openshift-ci-robot assigned jerolimov Feb 4, 2021

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

jerolimov approved these changes Feb 4, 2021

View reviewed changes

openshift-merge-robot merged commit a89f2a0 into openshift:master Feb 5, 2021

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

rottencandy deleted the pipeline-icon-spin branch February 8, 2021 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1923721: Handle pipeline svg icon spin using a wrapped <g> element #8007

Bug 1923721: Handle pipeline svg icon spin using a wrapped <g> element #8007

rottencandy commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

rottencandy commented Feb 1, 2021

karthikjeeyar commented Feb 2, 2021

jerolimov Feb 2, 2021 •

edited

rottencandy Feb 2, 2021

jerolimov commented Feb 2, 2021

nemesis09 commented Feb 3, 2021

andrewballantyne Feb 4, 2021 •

edited

rottencandy Feb 4, 2021 •

edited

jerolimov Feb 4, 2021

rottencandy Feb 4, 2021

andrewballantyne left a comment

jerolimov commented Feb 4, 2021

jerolimov left a comment

openshift-ci-robot commented Feb 4, 2021

christianvogt commented Feb 4, 2021

openshift-bot commented Feb 4, 2021

openshift-ci-robot commented Feb 5, 2021

openshift-ci bot commented Feb 5, 2021

Bug 1923721: Handle pipeline svg icon spin using a wrapped <g> element #8007

Bug 1923721: Handle pipeline svg icon spin using a wrapped <g> element #8007

Conversation

rottencandy commented Feb 1, 2021

openshift-ci-robot commented Feb 1, 2021

rottencandy commented Feb 1, 2021

karthikjeeyar commented Feb 2, 2021

jerolimov Feb 2, 2021 • edited

Choose a reason for hiding this comment

rottencandy Feb 2, 2021

Choose a reason for hiding this comment

jerolimov commented Feb 2, 2021

nemesis09 commented Feb 3, 2021

andrewballantyne Feb 4, 2021 • edited

Choose a reason for hiding this comment

rottencandy Feb 4, 2021 • edited

Choose a reason for hiding this comment

jerolimov Feb 4, 2021

Choose a reason for hiding this comment

rottencandy Feb 4, 2021

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

jerolimov commented Feb 4, 2021

jerolimov left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 4, 2021

christianvogt commented Feb 4, 2021

openshift-bot commented Feb 4, 2021

openshift-ci-robot commented Feb 5, 2021

openshift-ci bot commented Feb 5, 2021

jerolimov Feb 2, 2021 •

edited

andrewballantyne Feb 4, 2021 •

edited

rottencandy Feb 4, 2021 •

edited