Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Bug 1780338: fix(che-icon): add che Icon in the topology #3629

Merged
merged 1 commit into from Dec 6, 2019
Merged

Bug 1780338: fix(che-icon): add che Icon in the topology #3629

merged 1 commit into from Dec 6, 2019

Conversation

karthikjeeyar
Copy link
Contributor

If the eclipse che operator is installed and configured, then show the che icon instead of git source icon.
Topology · OKD

AwesomeScreenshot-2019-12-2-1575302155744

Fixes: https://jira.coreos.com/browse/ODC-2383

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin labels Dec 2, 2019
christianvogt
christianvogt reviewed Dec 2, 2019
View reviewed changes
frontend/packages/dev-console/src/components/topology/shapes/CheIcon.tsx Outdated
Comment on lines 16 to 35
<SVGDefs id={FILTER_ID}>
<filter id={FILTER_ID}>
<feOffset result="nw" in="SourceAlpha" dx="-0.5" dy="-0.5" />
<feOffset result="ne" in="SourceAlpha" dx="0.5" dy="-0.5" />
<feOffset result="se" in="SourceAlpha" dx="0.5" dy="0.5" />
<feOffset result="sw" in="SourceAlpha" dx="-0.5" dy="0.5" />
<feMerge result="blackborder">
<feMergeNode in="nw" />
<feMergeNode in="ne" />
<feMergeNode in="se" />
<feMergeNode in="sw" />
</feMerge>
<feFlood floodColor="#FFFFFF" />
<feComposite in2="blackborder" operator="in" result="offsetColor" />
<feMerge>
<feMergeNode in="offsetColor" />
<feMergeNode in="SourceGraphic" />
</feMerge>
</filter>
</SVGDefs>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose here? none of our other icons do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was adding white background to the che icon, but since the decorator base color is also white, i have removed it.

frontend/public/imgs/logos/che.svg Outdated
Comment on lines 5 to 13
<metadata>
<rdf:RDF>
<cc:Work rdf:about="">
<dc:format>image/svg+xml</dc:format>
<dc:type rdf:resource="http://purl.org/dc/dcmitype/StillImage"/>
<dc:title/>
</cc:Work>
</rdf:RDF>
</metadata>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be removed from the icon.

frontend/public/imgs/logos/che.svg Outdated
<path d="M0.032227,30.88l-0.032227-17.087,23.853-13.793,23.796,13.784-14.691,8.51-9.062-5.109-23.864,13.695z" fill="#fdb940"/>
<path d="M0,43.355l23.876,13.622,23.974-13.937v-16.902l-23.974,13.506-23.876-13.506v17.217z" fill="#525c86"/>
</g>
</svg>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing line at EOF

frontend/public/components/catalog/catalog-item-icon.tsx Outdated
@@ -102,6 +103,7 @@ const logos = new Map()
.set('icon-capedwarf', capedwarfImg)
.set('icon-catalog', catalogImg)
.set('icon-cassandra', cassandraImg)
.set('icon-che', cheImg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer that we put the icon in our own package for now as it's not part of the catalog.

frontend/packages/dev-console/src/components/topology/topology-types.ts
@@ -100,6 +100,7 @@ export interface ConnectedWorkloadPipeline {
export interface WorkloadData {
url?: string;
editUrl?: string;
cheEnabled?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we're better off having a cheUrl here and in the react component check if cheUrl is present then use it and the che icon, else do the status quo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

editUrl is already determining the correct url to pick, this cheEnabled variable is used only to pick the right icons.

sahil143
sahil143 approved these changes Dec 2, 2019
View reviewed changes
Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verified changes locally.

@debsmita1
Copy link
Contributor

tested the changes locally, was able to see the Che icon
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2019
christianvogt
christianvogt reviewed Dec 3, 2019
View reviewed changes
frontend/packages/dev-console/src/components/topology2/components/nodes/WorkloadNode.tsx Outdated
Comment on lines 54 to 57
x={cx - radius * 1.07}
y={cx - radius * 1.02}
width={radius * 0.39}
height={radius * 0.31}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why all these magic numbers?
The PF icons have a height and width of 1em so we control them using the font-size and set it to the radius.
I would expect the width and height here to also be the radius and the che svg itself be adjusted to fit appropriately similar to the PF icons.
The fact that the width and height aren't the same here means the icon is distorted no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christianvogt The icon is not distorted , i have updated the code to have width and height as 1em and adjusting the cheIcon now using the fontsize similar to the PF Icons.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2019
@karthikjeeyar
Copy link
Contributor Author

/retitle Bug 1780338: fix(che-icon): add che Icon in the topology

@openshift-ci-robot openshift-ci-robot changed the title add che Icon in the topology Bug 1780338: fix(che-icon): add che Icon in the topology Dec 5, 2019
@openshift-ci-robot
Copy link
Contributor

@karthikjeeyar: This pull request references Bugzilla bug 1780338, which is invalid:

  • expected the bug to target the "4.4.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1780338: fix(che-icon): add che Icon in the topology

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 openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 5, 2019
@karthikjeeyar karthikjeeyar changed the title Bug 1780338: fix(che-icon): add che Icon in the topology fix(che-icon): add che Icon in the topology Dec 5, 2019
@openshift-ci-robot openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 5, 2019
@openshift-ci-robot
Copy link
Contributor

@karthikjeeyar: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

fix(che-icon): add che Icon in the topology

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.

christianvogt
christianvogt reviewed Dec 5, 2019
View reviewed changes
frontend/packages/dev-console/src/components/topology/shapes/CheIcon.tsx Outdated
);
};

export default React.memo(CheIcon);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memo is fairly useless here because style is passed in as a new object each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it.

@christianvogt
Copy link
Contributor

/approve

make the small fix and have someone else give a lgtm

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2019
@christianvogt
Copy link
Contributor

/retitle Bug 1780338: fix(che-icon): add che Icon in the topology

@openshift-ci-robot openshift-ci-robot changed the title fix(che-icon): add che Icon in the topology Bug 1780338: fix(che-icon): add che Icon in the topology Dec 5, 2019
@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Dec 5, 2019
@openshift-ci-robot
Copy link
Contributor

@karthikjeeyar: This pull request references Bugzilla bug 1780338, which is valid. The bug has been moved to the POST state.

In response to this:

Bug 1780338: fix(che-icon): add che Icon in the topology

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.

@invincibleJai
Copy link
Member

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Dec 6, 2019
@invincibleJai
Copy link
Member

invincibleJai commented Dec 6, 2019 •
edited

/lgtm

Verified locally seems good, was able to see che icon however on click of that che opens up in new tab it says authorization token missing (it's not related to this PR could be an issue with CHE) cc @rohitkrai03 @karthikjeeyar

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, debsmita1, invincibleJai, karthikjeeyar, sahil143

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:

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

@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@karthikjeeyar
Copy link
Contributor Author

karthikjeeyar commented Dec 6, 2019 •
edited

/lgtm

Verified locally seems good, was able to see che icon however on click of that che opens up in new tab it says authorization token missing (it's not related to this PR could be an issue with CHE) cc @rohitkrai03 @karthikjeeyar

@invincibleJai we need to register the user to che workspace by clicking on the che(deployment) route icon for the first time, an authentication token will be generated. Once it is generated this error will not occur.

@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@karthikjeeyar
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit f0b0943 into openshift:master Dec 6, 2019
@openshift-ci-robot
Copy link
Contributor

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

In response to this:

Bug 1780338: fix(che-icon): add che Icon in the topology

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.

@karthikjeeyar
Copy link
Contributor Author

/cherrypick release-4.3

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Dec 9, 2019
Merged
@openshift-cherrypick-robot
Copy link

@karthikjeeyar: new pull request created: #3721

In response to this:

/cherrypick release-4.3

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.

@spadgett spadgett added this to the v4.4 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christianvogt christianvogt christianvogt left review comments

@sahil143 sahil143 sahil143 approved these changes

@debsmita1 debsmita1 Awaiting requested review from debsmita1

@jcaianirh jcaianirh Awaiting requested review from jcaianirh

Assignees

@invincibleJai invincibleJai

@debsmita1 debsmita1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@karthikjeeyar @debsmita1 @openshift-ci-robot @christianvogt @invincibleJai @openshift-bot @openshift-cherrypick-robot @sahil143 @spadgett @openshift-merge-robot