Bug 1804285: Fix for overlapping labels in topology nodes #4354
Conversation
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@jeff-phillips-18: This pull request references Bugzilla bug 1804285, 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. In response to this:
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
size/XS
| @@ -18,7 +18,7 @@ const LEFT_MARGIN = 20; | |||
| const TEXT_MARGIN = 10; | |||
|
|
|||
| const truncateOptions: TruncateOptions = { | |||
| length: 35, | |||
| length: 20, | |||
There was a problem hiding this comment.
Noticed that in some cases the name of groups would overlap the group boundaries as well. Changes the truncation length to be the standard 20 characters.
|
There are changes in the truncation that still causes the labels to be large:
Truncation not occurring until a certain threshold is hit: |
jeff-phillips-18
force-pushed
the
group-labels
branch
from
34f983f
to
1336574
Compare
openshift-ci-robot
added
size/M
|
@christianvogt Updated:
|
jeff-phillips-18
force-pushed
the
group-labels
branch
from
1336574
to
0954578
Compare
openshift-ci-robot
added
size/L
|
/cherrypick release-4.4 |
|
@jeff-phillips-18: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you. In response to this:
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. |
jeff-phillips-18
force-pushed
the
group-labels
branch
from
0954578
to
03e2c95
Compare
|
/test analyze |
|
/kind bug |
openshift-ci-robot
added
the
kind/bug
| @@ -115,10 +108,34 @@ class LayoutNode { | |||
| this.yy = undefined; | |||
| } | |||
|
|
|||
| @computed | |||
There was a problem hiding this comment.
I don't think this needs to be a @computed value because bounds is constantly changing during layout.
Did you do this for performance and find that a memoized value was beneficial?
There was a problem hiding this comment.
No, I just assumed that with cloning the performance would be better if we set @computed.
| return ( | ||
| this.node | ||
| .getBounds() | ||
| .clone() | ||
| .padding(this.node.getStyle<GroupStyle>().padding).width + | ||
| this.distance * 2 | ||
| ); |
There was a problem hiding this comment.
We should avoid cloning if there is no padding.
| @@ -112,6 +112,13 @@ export default class Rect implements Translatable { | |||
| return this; | |||
| } | |||
|
|
|||
| // | |||
| // Padding Format: [all], [horizontal, vertical], [left, vertical, right], [left, top, right, bottom] | |||
There was a problem hiding this comment.
Not sure what I was thinking when I did this.
Let's update to follow the CSS shorthand notation.
[all], [vertical, horizontal], [top, horizontal, bottom], [top, right, bottom, left]
| const b = c | ||
| .getBounds() | ||
| .clone() | ||
| .padding(c.getStyle<GroupStyle>().padding); |
There was a problem hiding this comment.
Best to avoid cloning as much as possible especially if there is no padding.
| this.node | ||
| .getBounds() | ||
| .clone() | ||
| .padding(this.node.getStyle<GroupStyle>().padding).height + |
There was a problem hiding this comment.
Let's rename the type from GroupStyle to NodeStyle
|
/bugzilla refresh |
|
@spadgett: This pull request references Bugzilla bug 1804285, which is valid. In response to this:
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. |
jeff-phillips-18
force-pushed
the
group-labels
branch
2 times, most recently
from
3768bd8
to
5ddd453
Compare
|
/retest |
1 similar comment
|
/retest |
jeff-phillips-18
force-pushed
the
group-labels
branch
from
5ddd453
to
e497cf5
Compare
|
/retest |
1 similar comment
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: christianvogt, jeff-phillips-18, serenamarie125 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 |
openshift-ci-robot
added
the
approved
|
@jeff-phillips-18: All pull requests linked via external trackers have merged. Bugzilla bug 1804285 has been moved to the MODIFIED state. In response to this:
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. |
|
@jeff-phillips-18: new pull request created: #4451 In response to this:
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. |
Fixes:
https://issues.redhat.com/browse/ODC-3035
Analysis / Root cause:
The change was made to be 20 characters when we added the name of the resource to the edit action in the context menu so they would be consistent. 20 characters is what the rest of the Console UI uses and we wanted consistency.
Solution Description:
Change both the nodes and the context menu to truncate at 16 characters.
Browser conformance:
cc @openshift/team-devconsole-ux @Veethika @serenamarie125