Bug 1921610: fix pipeline metrics font size issue #7961
Conversation
openshift-ci-robot
added
the
kind/bug
karthikjeeyar
changed the title
openshift-ci-robot
added
bugzilla/severity-low
|
@karthikjeeyar: This pull request references Bugzilla bug 1921610, 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
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. |
| hiddenSeries={hiddenSeries} | ||
| tickValues={tickValues} | ||
| width={contentRect.bounds.width} | ||
| height={chartHeight} |
There was a problem hiding this comment.
can we even get height from contentRect.bounds.height
There was a problem hiding this comment.
This is the last chart in the bottom row and this one has interactive legends under the chart, whereas the other chart in the bottom row doesn't have this legends. So fixed Height will make the cards even, if i use a dynamic height the cards wont be on the same size.
|
/approve Verified the changes , works as expected |
openshift-ci-robot
added
the
approved
| expect(PipelineRunCountWrapper.find(TimeSeriesChart).exists()).toBe(true); | ||
| expect(PipelineRunCountWrapper.find(LoadingInline).exists()).toBe(false); | ||
| expect(PipelineRunCountWrapper.find(GraphEmpty).exists()).toBe(false); | ||
| expect(PipelineRunCountWrapper.find(Measure).exists()).toBe(true); |
There was a problem hiding this comment.
@karthikjeeyar Is this change really necessary? The change of this tests "... if data is available" doesn't check that there is a TimeSeriesChart anymore.
I would like to see that this test case still tests for the TimeSeriesChart, or? Wdyt?
There was a problem hiding this comment.
@karthikjeeyar looks like you need to go two levels deeper with the shallow-renderer to find your TimeSeriesChart again. One for the <Measure /> and one for the children-render-function:
expect(
PipelineRunCountWrapper.dive()
.dive()
.find(TimeSeriesChart)
.exists(),
).toBe(true);
Want add this test again? Up to you if you want also keep the other checks. With the Measure check you test some internal which are not required in my opionion.
There was a problem hiding this comment.
Agreed @jerolimov. I will update the tests and push again. Thanks for pointing out.
karthikjeeyar
force-pushed
the
pipeline-metrics-font
branch
from
90ed422
to
701d6f4
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bgliwa01, invincibleJai, jerolimov, karthikjeeyar 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 |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/override ci/prow/ceph-storage-plugin The ceph-storage-plugin job should only have run on PRs with changes to |
|
@spadgett: Overrode contexts on behalf of spadgett: ci/prow/ceph-storage-plugin 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. |
|
@karthikjeeyar: All pull requests linked via external trackers have merged: Bugzilla bug 1921610 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. |
Fixes:
https://issues.redhat.com/browse/ODC-5312
Analysis / Root cause:
Font size in the chart labels should be consistent while resizing the window to a smaller screen.
Solution Description:
Used resize Observer (react measure) to pass the container width to PF charts.
Screen shots / Gifs for design review:
Test setup:
Browser conformance:
/kind bug
cc: @andrewballantyne @christianvogt @bgliwa01