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.

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

fix: replaced helm png by svg, use resized, not scaled icon (#874) #920

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

adietish
Copy link
Contributor

fixes #874

@adietish adietish self-assigned this Sep 27, 2024
@openshift-ci openshift-ci bot requested a review from sbouchet September 27, 2024 15:09
@adietish adietish changed the title replaced helm png by svg, provided 16px and 32px varian (#874) fix: replaced helm png by svg, provided 16px and 32px varian (#874) Sep 27, 2024
@adietish
Copy link
Contributor Author

adietish commented Sep 27, 2024

@sbouchet:
The icon used here has a size of 32x32. When using it for the tree node, it gets scaled to 16x16.
Text and Icons are provided to a node in a so called "Presentation". When a node is selected, the presentation is updated. Weird enough, this updated presentation is using the unscaled/original size of the icon (not the scaled version that it was given when the node was created).
I thus provide 2 different svg icons (replacing the original png): a 32x32 variant and a 16x16 variant. The 16x16 variant is to be used in the tree, the 32x32 is to be used in the helm charts dialog.
Please review. Thx.

@sbouchet
Copy link
Collaborator

there is still a wrong size for installed charts, when selected :
image

@adietish
Copy link
Contributor Author

adietish commented Sep 30, 2024

@sbouchet: I suspect that this is caused by a bug in the IDEA platform: we were using "scaled" icons (32px original size, scaled to 16px in the tree, unscaled 32px in the charts dialog). We do this using SwingUtils#scaleIcon. A scaled icon keeps its original size but indicates a scaled "display" size (I tracked things down to the tree cell renderer where it paints a selected node). The tree cell renderer is using the scaled display size when rendering a non-selected tree node, but is using the original size when rendering a selected node. There's no such problem when using an new icon with the corrected size using IconUtil#resizeSquared.

@adietish
Copy link
Contributor Author

@sbouchet: please re-review

Copy link

@adietish
Copy link
Contributor Author

adietish commented Oct 1, 2024

/override ci/prow/e2e-openshift

Copy link

openshift-ci bot commented Oct 1, 2024

@adietish: Overrode contexts on behalf of adietish: ci/prow/e2e-openshift

In response to this:

/override ci/prow/e2e-openshift

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-sigs/prow repository.

@adietish adietish changed the title fix: replaced helm png by svg, provided 16px and 32px varian (#874) fix: replaced helm png by svg, use resized, not scaled icon (#874) Oct 1, 2024
Copy link
Collaborator

@sbouchet sbouchet left a comment

Choose a reason for hiding this comment

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

tested against new UI and Old UI, everything is correct !

Copy link

openshift-ci bot commented Oct 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbouchet

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-ci openshift-ci bot added the approved label Oct 1, 2024
@adietish
Copy link
Contributor Author

adietish commented Oct 2, 2024

/override "Aggregated Test Report"

Copy link

openshift-ci bot commented Oct 2, 2024

@adietish: Overrode contexts on behalf of adietish: Aggregated Test Report

In response to this:

/override "Aggregated Test Report"

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-sigs/prow repository.

@adietish
Copy link
Contributor Author

adietish commented Oct 2, 2024

/override "Cluster Integration UI Tests Report"

Copy link

openshift-ci bot commented Oct 2, 2024

@adietish: Overrode contexts on behalf of adietish: Cluster Integration UI Tests Report

In response to this:

/override "Cluster Integration UI Tests Report"

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-sigs/prow repository.

@adietish
Copy link
Contributor Author

adietish commented Oct 2, 2024

/override "Public Integration UI Tests Report"

Copy link

openshift-ci bot commented Oct 2, 2024

@adietish: Overrode contexts on behalf of adietish: Public Integration UI Tests Report

In response to this:

/override "Public Integration UI Tests Report"

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-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 90bf911 into redhat-developer:main Oct 2, 2024
23 of 24 checks passed
@adietish adietish deleted the issue-874 branch October 2, 2024 20:27
sbouchet pushed a commit to sbouchet/intellij-openshift-connector that referenced this pull request Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart icons increase in size when highlighted with New UI
2 participants