Bug 1835232: Added skeleton loading state for data consumption and object data reduction card #3836

afreen23 · 2019-12-27T13:55:04Z

bipuladh

A few minor comments overall it looks good.

bipuladh · 2019-12-31T14:04:08Z

...ackages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.tsx

+            />
+          }
+          minDomain={{ y: 0 }}
+          maxDomain={{ y: maxVal + 10 }}


Please use proper statistical quartile.

What do you suggest here ?
Showing plus 10 of max value seems good enough.

The 3rd quartile.

Why we should be using that ? Please explain

how are we determining the maxVal? The third quartile is suitable for a tick point but not for the maxDomain. Basically you want the graph to be larger than the value by some factor. For large values +10 would not make a difference. Maybe something like. maxVal + maxVal*.25

maxVal if you check is the highest of all datapoints and its a humanized value.

Exactly this is not for tickpoints, but to have some extra space on axis for enhancing the look of graph.Typically above max value.

I like the suggestion here wrt maxVal + maxVal*.25 just one minor thing is that it will provide some fraction values and that can mess up in certain situations. (reason: d3 scale uses max domain to plot ticks and counts of ticks , and a fractional max domain does not comes up well, what I have tested)
I think (maxVal + Math.round(maxVal*.25)) would improve and save.

3 sounds like the way to go. Thanks for the info. Dint know about the issue.

...ackages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.tsx

bipuladh · 2020-01-24T12:44:40Z

...ackages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.tsx

+      </>
+    );
+  } else {
+    body = <GraphEmpty />;


put this as initial value of body

We want to show loading states first and this is rendered in case of errors.

I see no problem with it. If the initial value is graphyEmpty. And if it is loading then that should get changed.

I would not prefer overriding :)

Technically you are still overriding with the else statement. This would result in you not requiring else statement.

Think about it, I dont think so :)

Maybe I am missing something. LGTM. :)

openshift-ci-robot · 2020-05-13T13:24:03Z

@afreen23: This pull request references Bugzilla bug 1835232, 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

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1835232: Added skeleton loading state for data consumption and object data reduction card

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.

bipuladh · 2020-05-13T14:13:00Z

...ackages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.tsx

+          />
+          <ChartGroup offset={sortByKpi === BY_EGRESS ? 0 : 11}>
+            {chartData.map((data, i) => (
+              <ChartBar key={`chartbar-${i}`} data={data} /> // eslint-disable-line react/no-array-index-key


Suggested change

<ChartBar key={`chartbar-${i}`} data={data} /> // eslint-disable-line react/no-array-index-key

<ChartBar key={`chartbar-${i}`} data={data} />

No need to disable the rule

Required, since its still using array index.
May be we can refactor it in 4.6 to send in the indexes in chartdata.
Lets keep the PR focused to what bug its trying to solve.

Sure. But I have passed keys like the way you have passed it in several places and it dint give me that linter error. But if you're facing the issue then we need to disable it. No issues. :)

bipuladh · 2020-05-17T03:40:05Z

/lgtm cancel
@afreen23 lets resolve #3836 (comment) and then I will put the tag back on. Sorry for the confusion.

bipuladh · 2020-05-18T07:43:17Z

...baa-storage-plugin/src/components/object-data-reduction-card/object-data-reduction-card.scss

@@ -22,3 +22,7 @@
 .nb-object-data-reduction-card__row-title {
  margin-right: 0.5em;
 }
+


Can we get rid of line 1? I don't think it's being used anywhere.

…uction card Signed-off-by: Afreen Rahman <afrahman@redhat.com>

bipuladh · 2020-05-18T10:10:13Z

/lgtm

openshift-ci-robot · 2020-05-18T10:10:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, bipuladh

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:

~~frontend/packages/noobaa-storage-plugin/OWNERS~~ [afreen23,bipuladh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-05-18T10:13:41Z

@afreen23: All pull requests linked via external trackers have merged: openshift/console#3836. Bugzilla bug 1835232 has been moved to the MODIFIED state.

In response to this:

Bug 1835232: Added skeleton loading state for data consumption and object data reduction card

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 size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/noobaa Related to noobaa-storage-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 27, 2019

openshift-ci-robot requested review from cloudbehl and rawagner December 27, 2019 13:55

afreen23 changed the title ~~Added skeleton loading state for data consumption~~ [WIP] Added skeleton loading state for data consumption Jan 3, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 3, 2020

afreen23 changed the title ~~[WIP] Added skeleton loading state for data consumption~~ Added skeleton loading state for data consumption Jan 24, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2020

bipuladh reviewed Jan 24, 2020

View reviewed changes

afreen23 changed the title ~~Added skeleton loading state for data consumption~~ Added skeleton loading state for data consumption and object data reduction card May 13, 2020

afreen23 force-pushed the add-skeleton-loaders-data-consumption branch from 728869a to 160fea4 Compare May 13, 2020 12:08

afreen23 changed the title ~~Added skeleton loading state for data consumption and object data reduction card~~ Bug 1835232: Added skeleton loading state for data consumption and object data reduction card May 13, 2020

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label May 13, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 13, 2020

bipuladh reviewed May 13, 2020

View reviewed changes

openshift-ci-robot assigned bipuladh May 17, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 17, 2020

afreen23 force-pushed the add-skeleton-loaders-data-consumption branch from 160fea4 to d004c69 Compare May 18, 2020 07:39

bipuladh reviewed May 18, 2020

View reviewed changes

Added skeleton loading state for data consumption and object data red…

f94ac63

…uction card Signed-off-by: Afreen Rahman <afrahman@redhat.com>

afreen23 force-pushed the add-skeleton-loaders-data-consumption branch from d004c69 to f94ac63 Compare May 18, 2020 07:53

bipuladh approved these changes May 18, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2020

openshift-merge-robot merged commit 81662da into openshift:master May 18, 2020

spadgett added this to the v4.5 milestone May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1835232: Added skeleton loading state for data consumption and object data reduction card #3836

Bug 1835232: Added skeleton loading state for data consumption and object data reduction card #3836

afreen23 commented Dec 27, 2019 •

edited

bipuladh left a comment

bipuladh Dec 31, 2019

afreen23 May 13, 2020

bipuladh May 13, 2020

afreen23 May 13, 2020 •

edited

bipuladh May 13, 2020

afreen23 May 16, 2020 •

edited

bipuladh May 17, 2020

bipuladh Jan 24, 2020

afreen23 May 13, 2020

bipuladh May 13, 2020

afreen23 May 13, 2020

bipuladh May 13, 2020

afreen23 May 16, 2020

bipuladh May 17, 2020 •

edited

openshift-ci-robot commented May 13, 2020

bipuladh May 13, 2020

afreen23 May 16, 2020

bipuladh May 17, 2020

bipuladh commented May 17, 2020

bipuladh May 18, 2020

afreen23 May 18, 2020

bipuladh commented May 18, 2020

openshift-ci-robot commented May 18, 2020

openshift-ci-robot commented May 18, 2020

	<ChartBar key={`chartbar-${i}`} data={data} /> // eslint-disable-line react/no-array-index-key
	<ChartBar key={`chartbar-${i}`} data={data} />

Bug 1835232: Added skeleton loading state for data consumption and object data reduction card #3836

Bug 1835232: Added skeleton loading state for data consumption and object data reduction card #3836

Conversation

afreen23 commented Dec 27, 2019 • edited

bipuladh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 May 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 May 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh May 17, 2020 • edited

Choose a reason for hiding this comment

openshift-ci-robot commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh commented May 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bipuladh commented May 18, 2020

openshift-ci-robot commented May 18, 2020

openshift-ci-robot commented May 18, 2020

afreen23 commented Dec 27, 2019 •

edited

afreen23 May 13, 2020 •

edited

afreen23 May 16, 2020 •

edited

bipuladh May 17, 2020 •

edited