Bug 2168484: Add dates to VM metric charts labels #1171
Conversation
|
@pcbailey: This pull request references Bugzilla bug 2168484, which is invalid:
Comment 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. |
| @@ -3,36 +3,32 @@ import React from 'react'; | |||
| import { V1VirtualMachineInstance } from '@kubevirt-ui/kubevirt-api/kubevirt'; | |||
| import MigrationThresholdChart from '@kubevirt-utils/components/Charts/MigrationUtil/MigrationThresholdChart'; | |||
| import MigrationThresholdChartDiskRate from '@kubevirt-utils/components/Charts/MigrationUtil/MigrationThresholdChartDiskRate'; | |||
| import { useKubevirtTranslation } from '@kubevirt-utils/hooks/useKubevirtTranslation'; | |||
| import { t } from '@kubevirt-utils/hooks/useKubevirtTranslation'; | |||
There was a problem hiding this comment.
When I first started looking at the bug I thought I was going to be changing this file and went ahead and made this change to make it look cleaner. Is it a problem? They should be equivalent, right?
There was a problem hiding this comment.
yes its equal, but until now we preferred using the hook method on tsx files and simply t on ts files, to keep it organized
There was a problem hiding this comment.
Hrm. That's the first I've heard of that. How is unnecessary boilerplate more organized?
There was a problem hiding this comment.
Simply as a hook for tsx and t for ts because u cannot use hooks.
Why do we not simply use t instead of a hook also for tsx?
t will be static and not rerender like t from a hook; how much its import?
I don't know... but this is why we kept it like this so if a new reactive version of t will be needed for some reason the hook will make sure of it.
maybe static t will cause problems in static utils files in the future? I also dont know, but we can always go back to re adding t as arg to function. hope this makes sense to u
There was a problem hiding this comment.
Sure. The short answer is "we don't know, but better safe than sorry". lol
I'll change it back in a few.
I don't know when this convention was discussed, but I don't remember being told about it. We need to make sure that information is disseminated clearly to the whole team, especially if it's something that's been discussed in person in the office.
pcbailey
force-pushed
the
add-dates-to-vm-metrics-charts
branch
from
438c590
to
848fd0f
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: metalice, pcbailey 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 |
|
@pcbailey: All pull requests linked via external trackers have merged: Bugzilla bug 2168484 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. |
馃摑 Description
This PR adds dates to the x-axis labels of the VM metric charts for durations that span multiple days and changes the time format for durations that do not.
馃帴 Screenshots
Multiple days format
Single day format