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.

Sign up for GitHub

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

Jump to bottom

feat: added the options to select via pvc and namespace for the breakdown card #905

Merged
merged 1 commit into from
Jul 4, 2023
Merged

feat: added the options to select via pvc and namespace for the breakdown card #905

merged 1 commit into from
Jul 4, 2023

Conversation

debjyoti-pandit
Copy link
Contributor

@debjyoti-pandit debjyoti-pandit commented Jun 21, 2023 •
edited
Loading

solves: https://issues.redhat.com/browse/RHSTOR-3927

initial on-page load

image

initial on pvc selected but there is no PVC in the default namespace. and by default, it means the namespace which is the first of the namespace list alphabetically

image

when there is a PVC in the selected namespace

image

tooltip
image

dropdown
image

photos updated on 3rd July 16:42 force pushed

@debjyoti-pandit
Copy link
Contributor Author

/assign @bipuladh
/assign @SanjalKatiyar

@debjyoti-pandit debjyoti-pandit force-pushed the RHSTOR-3927 branch 2 times, most recently from a6648e9 to 89a8cad Compare June 23, 2023 03:43
SanjalKatiyar
SanjalKatiyar reviewed Jun 26, 2023
View reviewed changes
packages/ocs/dashboards/persistent-internal/capacity-breakdown-card/capacity-breakdown-card.tsx
Comment on lines +135 to 142
<CardTitle id="breakdown-card-title">
{t('Requested capacity')}
<FieldLevelHelp testId="breakdown-card-helper-text">
{t(
'This card shows the used capacity for different Kubernetes resources. The figures shown represent the Usable storage, meaning that data replication is not taken into consideration.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Screenshot 2023-06-26 at 6 29 39 PM here "Requested capacity" has diff meaning than the one we are defining on this Card.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cc @bipuladh

Copy link
Collaborator

Choose a reason for hiding this comment

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

@debjyoti-pandit with UX once...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SanjalKatiyar I have tagged you in a IBM slack thread where discussed the same with UX.

Copy link
Collaborator

@SanjalKatiyar SanjalKatiyar Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I saw that... thanks for tagging again !
but that seems diff to me to what I pointed here... term "Requested capacity" have 2 diff meaning at 2 diff places, if that's okay with UX we are good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, didn't notice initially that you were part of the thread so tagged you again. and regarding the changes I am not quite clear on what are you suggesting.?

SanjalKatiyar
SanjalKatiyar reviewed Jun 26, 2023
View reviewed changes
packages/ocs/dashboards/persistent-internal/capacity-breakdown-card/capacity-breakdown-card.tsx
top5MetricsStats.length > 0 &&
cephUsed && (
<CardBody className="odf-capacity-breakdown-card-pvc-description">
{t('Only showing PVCs that are being mounted on an active pod')}
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is true for all options (pods/stprageclass/project/pvc)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I see in the UX, this is what is mentioned https://www.sketch.com/s/c43ed755-7832-4de4-84a0-4b4ce815f366/a/1KbRavk

bipuladh
bipuladh reviewed Jun 28, 2023
View reviewed changes
packages/shared/dashboards/breakdown-card/breakdown-dropdown.tsx Outdated Show resolved Hide resolved
packages/shared/dashboards/breakdown-card/breakdown-capacity.tsx Outdated Show resolved Hide resolved
packages/shared/dashboards/breakdown-card/breakdown-body.tsx Outdated Show resolved Hide resolved
packages/ocs/queries/ceph-storage.ts Outdated Show resolved Hide resolved
packages/ocs/dashboards/persistent-internal/capacity-breakdown-card/capacity-breakdown-card.tsx Outdated Show resolved Hide resolved
...ages/ocs/dashboards/persistent-internal/capacity-breakdown-card/capacity-breakdown-card.scss Outdated Show resolved Hide resolved
packages/ocs/dashboards/persistent-internal/capacity-breakdown-card/capacity-breakdown-card.tsx Outdated Show resolved Hide resolved
@debjyoti-pandit debjyoti-pandit force-pushed the RHSTOR-3927 branch 3 times, most recently from 3df5f4d to fd552bc Compare July 3, 2023 06:27
@debjyoti-pandit debjyoti-pandit force-pushed the RHSTOR-3927 branch 2 times, most recently from fd0c3f4 to c590b7e Compare July 3, 2023 10:46
@agarwal-mudit
Copy link
Member

@SanjalKatiyar @bipuladh can we merge this today?

@SanjalKatiyar
Copy link
Collaborator

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 4, 2023
@bipuladh
Copy link
Contributor

bipuladh commented Jul 4, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, debjyoti-pandit

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 Jul 4, 2023
@openshift-merge-robot openshift-merge-robot merged commit 092bf37 into red-hat-storage:master Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bipuladh bipuladh bipuladh left review comments

@SanjalKatiyar SanjalKatiyar SanjalKatiyar left review comments

Assignees

@SanjalKatiyar SanjalKatiyar

@bipuladh bipuladh

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@debjyoti-pandit @agarwal-mudit @SanjalKatiyar @bipuladh @openshift-merge-robot