Fix the time selection dropdown issue for the ceph RBD snapshot graph in DR dashboard #935
Conversation
| <GridItem | ||
| lg={10} | ||
| rowSpan={1} | ||
| sm={12} | ||
| className="pf-u-font-weight-bold pf-c-title pf-m-lg" | ||
| > | ||
| {t('Utilization')} | ||
| </GridItem> | ||
| <GridItem | ||
| lg={2} | ||
| rowSpan={1} | ||
| sm={12} | ||
| className="mco-dashboard__contentRow mco-cluster-app__contentRow--flexEnd" | ||
| > | ||
| <UtilizationDurationDropdown /> | ||
| </GridItem> |
There was a problem hiding this comment.
| <GridItem | |
| lg={10} | |
| rowSpan={1} | |
| sm={12} | |
| className="pf-u-font-weight-bold pf-c-title pf-m-lg" | |
| > | |
| {t('Utilization')} | |
| </GridItem> | |
| <GridItem | |
| lg={2} | |
| rowSpan={1} | |
| sm={12} | |
| className="mco-dashboard__contentRow mco-cluster-app__contentRow--flexEnd" | |
| > | |
| <UtilizationDurationDropdown /> | |
| </GridItem> |
There was a problem hiding this comment.
My suggestion is to create a new Component called UtilizationCard, call dropdown component, and other utilization-related components like SnapshotUtilizationCard. Call only UtilizationCard from here remove all other components
|
Your PR title is long and broken, also to link the BZ use format (Bug BZ_ID: PR title) |
|
plz fix commit title length... |
|
/retitle Bug 2223705: Fix the time selection dropdown issue for the ceph RBD snapshot graph in DR dashboard |
openshift-ci
bot
changed the title
|
@TimothyAsirJeyasing: This pull request references Bugzilla bug 2223705, which is valid. No validations were run on this bugRequesting review from QA contact: 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[bot]: GitHub didn't allow me to request PR reviews from the following users: PrasadDesala. Note that only red-hat-storage members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
TimothyAsirJeyasing
force-pushed
the
fix-dropdown-issue-drdashboard
branch
from
5c073a4
to
8fd630e
Compare
|
|
| @@ -318,6 +307,51 @@ export const ClusterAppCard: React.FC = () => { | |||
| ); | |||
| }; | |||
|
|
|||
| const UtilizationCard: React.FC<UtilizationCardProps> = ({ | |||
There was a problem hiding this comment.
Move this inside cluster.tsx
There was a problem hiding this comment.
Ok, if that is the case, shall i move the content of SnapshotUtilizationCard into UtilizationCard. Because the SnapshotUtilizationCard component is only used within the UtilizationCard component and doesn't have any additional reusability across the application, So shall i consider merging the logic of SnapshotUtilizationCard directly into UtilizationCard. Feeling like it can lead to a simpler component structure and reduce the number of components. ?
There was a problem hiding this comment.
UtilizationCard is a common one, SnapshotUtilizationCard is a DR specific. It will be used only inside DRCard.
| const { t } = useCustomTranslation(); | ||
|
|
||
| return ( | ||
| <Grid hasGutter> |
There was a problem hiding this comment.
don't use a grid or GridItem here use CSS to maintain a space between graphs.
| sm={12} | ||
| className="mco-dashboard__contentRow mco-cluster-app__contentRow--flexEnd" | ||
| > | ||
| <UtilizationDurationDropdown /> |
There was a problem hiding this comment.
you can still use some existing css for title and dropdown e.g: https://github.com/red-hat-storage/odf-console/pull/887/files#diff-def5abe8fbdb01c7ae03e68f2534b6c51ef54676572d0964817b4dc235ece10aL221
There was a problem hiding this comment.
Sure, will do that
| type UtilizationCardProps = { | ||
| clusterName: string; | ||
| peerClusters: string[]; | ||
| }; |
There was a problem hiding this comment.
movie this also inside cluster.tsx
TimothyAsirJeyasing
force-pushed
the
fix-dropdown-issue-drdashboard
branch
from
8fd630e
to
7b011e1
Compare
locales/en/plugin__odf-console.json
Outdated
| "Current value: ": "Current value: ", | ||
| "Max value: ": "Max value: ", | ||
| "Min value: ": "Min value: ", | ||
| "Utlization": "Utlization", |
There was a problem hiding this comment.
type Utlization => Utilization
| CustomUtilizationSummary, | ||
| }) => { | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
| <> | |
| showLegend | ||
| CustomUtilizationSummary={CustomUtilizationSummary} | ||
| /> | ||
| </> |
There was a problem hiding this comment.
| </> | |
| component={TextVariants.h3} | ||
| className="mco-cluster-app__contentRow--flexStart" | ||
| > | ||
| {t('Utlization')} |
There was a problem hiding this comment.
| {t('Utlization')} | |
| {t('Utilization')} |
| hideHorizontalBorder | ||
| showLegend | ||
| </div> | ||
| <div style={{ marginBottom: '20px' }}> |
There was a problem hiding this comment.
Don't use any style property directly. Also, use only PF global values.
There was a problem hiding this comment.
pf-m-spacer-* classes in PatternFly are not working for adding vertical spacing between elements. Worked only for components like buttons, form elements, and some others. So shall i use css to deal this ?
There was a problem hiding this comment.
you need to use a class from CSS and PF global variable works for vertical spacing, there is lot of reference in other places
| clusterName: string; | ||
| peerClusters: string[]; |
There was a problem hiding this comment.
| clusterName: string; | |
| peerClusters: string[]; | |
| peerClusters: string[]; |
use only peerClusters
There was a problem hiding this comment.
We need the selected cluster name for the Replication throughput ...
There was a problem hiding this comment.
you can pass the clusterName inside peerClusters array and fetch from it
TimothyAsirJeyasing
force-pushed
the
fix-dropdown-issue-drdashboard
branch
from
7b011e1
to
0a3e824
Compare
|
shorten commit title and fix build tests... |
| humanizeValue={humanizeDecimalBytesPerSec} | ||
| CustomUtilizationSummary={CustomUtilizationSummary} | ||
| /> | ||
| <UtilizationCard peerClusters={[...peerClusters, clusterName]} /> |
There was a problem hiding this comment.
Peer cluster is already has current cluster name, why we need to pass again?
There was a problem hiding this comment.
ok, so you need the current cluster for the throughput graph, ok the pass is as different argument. I think the old code was fine
| @@ -41,6 +41,10 @@ | |||
| height: auto; | |||
| } | |||
|
|
|||
| .mco-cluster-app__space-between { | |||
There was a problem hiding this comment.
| .mco-cluster-app__space-between { | |
| .mco-cluster-app__graph--margin-top { |
| @@ -41,6 +41,10 @@ | |||
| height: auto; | |||
| } | |||
|
|
|||
| .mco-cluster-app__space-between { | |||
| margin-top: 20px; | |||
There was a problem hiding this comment.
use --pf-global--spacer
TimothyAsirJeyasing
force-pushed
the
fix-dropdown-issue-drdashboard
branch
from
0a3e824
to
0d71110
Compare
|
/test odf-console-e2e-aws |
|
|
/cherry-pick release-4.14 |
|
@GowthamShanmugam: once the present PR merges, I will cherry-pick it on top of release-4.14 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. |
|
/retitle Fix the time selection dropdown issue for the ceph RBD snapshot graph in DR dashboard |
openshift-ci
bot
changed the title
|
/cherry-pick release-4.14-compatibility |
|
@GowthamShanmugam: once the present PR merges, I will cherry-pick it on top of release-4.14-compatibility 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. |
|
/lgtm cancel |
TimothyAsirJeyasing
force-pushed
the
fix-dropdown-issue-drdashboard
branch
from
0d71110
to
c8464b7
Compare
| <UtilizationDurationDropdown /> | ||
| </div> | ||
| </div> | ||
| <div> |
There was a problem hiding this comment.
Do we really need div here?
There was a problem hiding this comment.
I am using this for css class
| humanizeValue={humanizeNumber} | ||
| /> | ||
| </div> | ||
| <div className="mco-cluster-app__graph--margin-top"> |
There was a problem hiding this comment.
Instead of top use margin-below and use this class name in both SnapshotUtilizationCard div
TimothyAsirJeyasing
force-pushed
the
fix-dropdown-issue-drdashboard
branch
from
c8464b7
to
c6ca083
Compare
| @@ -41,6 +41,10 @@ | |||
| height: auto; | |||
| } | |||
|
|
|||
| .mco-cluster-app__graph--margin-below { | |||
There was a problem hiding this comment.
| .mco-cluster-app__graph--margin-below { | |
| .mco-cluster-app__graph--margin-bottom { |
Signed-off-by: Timothy Asir Jeyasingh <[email protected]>
TimothyAsirJeyasing
force-pushed
the
fix-dropdown-issue-drdashboard
branch
from
c6ca083
to
feb785a
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GowthamShanmugam, SanjalKatiyar, TimothyAsirJeyasing 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 |
|
/cherry-pick release-4.14 |
|
@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.14 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. |
|
/cherry-pick release-4.14-compatibility |
|
@SanjalKatiyar: once the present PR merges, I will cherry-pick it on top of release-4.14-compatibility 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. |
openshift-merge-robot
merged commit 559babd
into
red-hat-storage:master
|
@GowthamShanmugam: new pull request created: #976 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. |
|
@GowthamShanmugam: new pull request created: #977 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. |
Bug: 2223705