Refactor Capacity Card and extract CapacityBody and CapacityItem to D… #301

rawagner · 2019-04-02T17:54:31Z

…ashboard folder

rawagner · 2019-04-02T17:56:02Z

coveralls · 2019-04-02T17:58:21Z

Pull Request Test Coverage Report for Build 1004

45 of 50 (90.0%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.2%) to 87.253%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/components/Dashboard/Capacity/CapacityItem.js	28	29	96.55%
src/selectors/prometheus/selectors.js	4	8	50.0%

Totals
Change from base Build 1222:	-0.2%
Covered Lines:	3064
Relevant Lines:	3358

💛 - Coveralls

atiratree · 2019-04-03T08:19:37Z

src/components/Dashboard/Capacity/CapacityItem.js

+};
+
+export const CapacityItem = ({ idSuffix, title, used, total, unit, formatValue, LoadingComponent }) => {
+  if (!Number.isNaN(parseFloat(used)) && !Number.isNaN(parseFloat(total))) {


this component probably shouldn't do the parsing and expect numbers instead. We could maybe do this in getCapacityStats. Then we can check for null here

parsing moved to getCapacityStats and CapacityItem is checking for null

atiratree · 2019-04-03T08:21:05Z

src/components/Dashboard/Capacity/CapacityItem.js

+  }</span>`;
+
+const LoadingCapacityItem = ({ idSuffix, title, LoadingComponent }) => (
+  <div className="kubevirt-capacity__item">


this layout is already present in CapacityItem. Can you please put some ifs there and do the loading design there? Current approach would make the layout duplicated and harder to modify in the future.

atiratree · 2019-04-03T08:24:12Z

src/components/Dashboard/Capacity/CapacityItem.js

+        <h6>{`${availableNice} ${unit} available out of ${totalNice} ${unit}`}</h6>
+        <div>
+          <DonutChart
+            id={`donut-chart-${idSuffix}`}


I get that the idSuffix is useful (lazy approach). But I would prefer the id part was configurable as well. Can you either create idPrefix prop with donut-chart value as a default or add just an id prop which is more conventional?

also there is a function called prefixedId

prop renamed to id and Im using prefixedId now

atiratree · 2019-04-03T09:29:27Z

src/selectors/prometheus/selectors.js

+  if (Number.isNaN(parseFloat(value))) {
+    return null;
+  }
+  return Number(value);


Do we expect whitespaces in the value? Number is more stronger so it could still return NaN. We can just use the parseFloat value if we want to be more lenient.

Not sure about whitespaces...i would say no and stick with the Number for now

I just saw https://stackoverflow.com/questions/12227594/which-is-better-numberx-or-parsefloatx. Each one of them has it's quirks. Can we check for NaN of both parseFloat and Number ?

maybe it would be worth to also make a generic parsing function out of it. It would return number or null only.

I think a number-like value is returned, not a string

ok, still better to check against Number('') === 0

new function in utils - parseNumber is using combination of parseFloat/Number/Number.isNaN . WDYT ?

atiratree · 2019-04-03T09:29:33Z

src/components/Dashboard/Capacity/CapacityItem.js

+    tooltip = { contents: pfGetUtilizationDonutTooltipContents };
+    primaryTitle = `${percentageUsed.toString()}%`;
+    secondaryTitle = USED;
+  }


please move at least the description = <LoadingComponent /> to else branch

atiratree · 2019-04-03T09:38:47Z

src/components/Dashboard/Capacity/CapacityItem.js

+  let tooltip;
+  let primaryTitle = LOADING;
+  let secondaryTitle = CAPACITY;
+  if (used != null && total != null) {


hmm, on the secod look, I would rather pass isLoading from above and check that we already got the response (presence of response.data? in ClusterOverview/Capacity). This way the component can distinguish between three cases.

loading (no response - pass isLoading boolean)

loaded with wrong response (used/total is null) - we could show Not Available label for example

loaded correctly (show the data)

not sure about the prometheus format when there is no data yet or if prometheus can return invalid response.

@rawagner thoughts?

@suomiy I think that no data can happen if prometheus didnt scrape the endpoint yet. Not sure about the invalid response. Anyways I will add Not Available state too.

added isLoading prop and Not Available state.

atiratree · 2019-04-03T09:42:27Z

src/components/Dashboard/Capacity/CapacityItem.js

+      <h6>{description}</h6>
+      <div>
+        <DonutChart
+          id={prefixedId('donut-chart', id)}


I just didn't want to force the donut-chart part on the consumer. Nevertheless it probably doesn't matter that much.

atiratree · 2019-04-03T14:03:03Z

src/utils/utils.js

@@ -20,6 +20,8 @@ export const parseUrl = url => {
  }
 };

+export const parseNumber = value => (!Number.isNaN(parseFloat(value)) && !Number.isNaN(Number(value)) ? Number(value) : null);


we can reuse the result of Number(value)

…ashboard folder

kubevirt-bot added the size/XL label Apr 2, 2019

rawagner mentioned this pull request Apr 2, 2019

Capacity card refactored kubevirt/web-ui#254

Merged

atiratree suggested changes Apr 3, 2019

View reviewed changes

rawagner force-pushed the capacity_dashboard branch 2 times, most recently from 148df78 to f54cf49 Compare April 3, 2019 09:07

atiratree suggested changes Apr 3, 2019

View reviewed changes

atiratree reviewed Apr 3, 2019

View reviewed changes

rawagner force-pushed the capacity_dashboard branch 5 times, most recently from d4cf9ad to 6959d51 Compare April 3, 2019 13:50

atiratree reviewed Apr 3, 2019

View reviewed changes

Refactor Capacity Card and extract CapacityBody and CapacityItem to D…

fea3c33

…ashboard folder

rawagner force-pushed the capacity_dashboard branch from 6959d51 to fea3c33 Compare April 3, 2019 14:15

atiratree merged commit e98b86f into kubevirt:master Apr 3, 2019

Refactor Capacity Card and extract CapacityBody and CapacityItem to D… #301

Refactor Capacity Card and extract CapacityBody and CapacityItem to D… #301

Uh oh!

Conversation

rawagner commented Apr 2, 2019

Uh oh!

rawagner commented Apr 2, 2019

Uh oh!

coveralls commented Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1004

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atiratree Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Apr 2, 2019 •

edited

Loading

atiratree Apr 3, 2019 •

edited

Loading