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

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

Merged
merged 1 commit into from
May 18, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 11 additions & 0 deletions ...ges/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,14 @@
.nb-data-consumption-card__chart-label {
margin-top: var(--pf-global--spacer--md);
}

.nb-data-consumption-card__chart-skeleton {
margin-top: var(--pf-global--spacer--md);
margin-bottom: var(--pf-global--spacer--md);
width: 50%;
}

.nb-data-consumption-card__chart-legend-skeleton {
margin-top: var(--pf-global--spacer--md);
width: 80%;
}
127 changes: 69 additions & 58 deletions ...ages/noobaa-storage-plugin/src/components/data-consumption-card/data-consumption-card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const DataConsumptionCard: React.FC<DashboardItemProps> = ({
let suffixLabel = '';
let maxVal: number;
let maxUnit: string;
let body: React.ReactNode;

const isLoading = _.values(result).includes(undefined);

Expand All @@ -81,6 +82,73 @@ const DataConsumptionCard: React.FC<DashboardItemProps> = ({
if (suffixLabel) suffixLabel = `(in ${suffixLabel})`;
}

if (isLoading) {
body = (
<>
<div className="skeleton-text nb-data-consumption-card__chart-skeleton" />
<GraphEmpty height={200} loading />
<div className="skeleton-text nb-data-consumption-card__chart-legend-skeleton" />
</>
);
} else if (!error && !chartData.some(_.isEmpty)) {
body = (
<>
<div className="nb-data-consumption-card__chart-label text-secondary">
{CHART_LABELS[sortByKpi]} {suffixLabel}
</div>
<Chart
containerComponent={
<ChartVoronoiContainer
labelComponent={<ChartTooltip style={{ fontSize: 8, paddingBottom: 0 }} />}
labels={({ datum }) => `${datum.y} ${maxUnit}`}
voronoiDimension="x"
/>
}
minDomain={{ y: 0 }}
maxDomain={{ y: maxVal + Math.round(maxVal * 0.25) }}
domainPadding={{ x: [padding, padding] }}
legendComponent={
<ChartLegend
themeColor={ChartThemeColor.purple}
data={legendData}
orientation="horizontal"
symbolSpacer={5}
gutter={2}
height={30}
padding={{ top: 50, bottom: 0 }}
style={{ labels: { fontSize: 8 } }}
/>
}
legendPosition="bottom-left"
padding={{
bottom: 50,
left: 30,
right: 20,
top: 30,
}}
themeColor={ChartThemeColor.purple}
>
<ChartAxis style={{ tickLabels: { fontSize: 8, padding: 2 } }} />
<ChartAxis
dependentAxis
showGrid
tickCount={10}
style={{
tickLabels: { fontSize: 8, padding: 0 },
}}
/>
<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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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. :)

))}
</ChartGroup>
</Chart>
</>
);
} else {
body = <GraphEmpty />;
Copy link
Contributor

Choose a reason for hiding this comment

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

put this as initial value of body

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would not prefer overriding :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think about it, I dont think so :)

Copy link
Contributor

@bipuladh bipuladh May 17, 2020 •
edited

Choose a reason for hiding this comment

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

Maybe I am missing something. LGTM. :)

}

return (
<DashboardCard>
<DashboardCardHeader>
Expand All @@ -92,64 +160,7 @@ const DataConsumptionCard: React.FC<DashboardItemProps> = ({
setKpi={setsortByKpi}
/>
</DashboardCardHeader>
<DashboardCardBody className="co-dashboard-card__body--top-margin" isLoading={isLoading}>
{!error && !chartData.some(_.isEmpty) ? (
<>
<div className="nb-data-consumption-card__chart-label text-secondary">
{CHART_LABELS[sortByKpi]} {suffixLabel}
</div>
<Chart
containerComponent={
<ChartVoronoiContainer
labelComponent={<ChartTooltip style={{ fontSize: 8, paddingBottom: 0 }} />}
labels={({ datum }) => `${datum.y} ${maxUnit}`}
voronoiDimension="x"
/>
}
minDomain={{ y: 0 }}
maxDomain={{ y: maxVal + 10 }}
domainPadding={{ x: [padding, padding] }}
legendComponent={
<ChartLegend
themeColor={ChartThemeColor.purple}
data={legendData}
orientation="horizontal"
symbolSpacer={5}
gutter={2}
height={30}
padding={{ top: 50, bottom: 0 }}
style={{ labels: { fontSize: 8 } }}
/>
}
legendPosition="bottom-left"
padding={{
bottom: 50,
left: 30,
right: 20,
top: 30,
}}
themeColor={ChartThemeColor.purple}
>
<ChartAxis style={{ tickLabels: { fontSize: 8, padding: 2 } }} />
<ChartAxis
dependentAxis
showGrid
tickCount={10}
style={{
tickLabels: { fontSize: 8, padding: 0 },
}}
/>
<ChartGroup offset={sortByKpi === BY_EGRESS ? 0 : 11}>
{chartData.map((data, i) => (
<ChartBar key={i} data={data} /> // eslint-disable-line react/no-array-index-key
))}
</ChartGroup>
</Chart>
</>
) : (
<GraphEmpty />
)}
</DashboardCardBody>
<DashboardCardBody className="co-dashboard-card__body--top-margin">{body}</DashboardCardBody>
</DashboardCard>
);
};
Expand Down
7 changes: 3 additions & 4 deletions ...rage-plugin/src/components/object-data-reduction-card/object-data-reduction-card-item.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,15 @@ import {
FieldLevelHelp,
humanizeBinaryBytes,
humanizePercentage,
LoadingInline,
} from '@console/internal/components/utils';

const ItemBody: React.FC<ItemBodyProps> = React.memo(
({ title, stats, infoText, isLoading, error }) => {
let status: React.ReactElement;
if (error || !stats) {
if (isLoading) {
status = <div className="skeleton-text nb-object-data-reduction__item-body--loading" />;
} else if (error || !stats) {
status = <span className="co-dashboard-text--small text-muted">Not available</span>;
} else if (isLoading) {
status = <LoadingInline />;
} else {
status = <span className="nb-object-data-reduction-card__row-status-item-text">{stats}</span>;
}
Expand Down
6 changes: 4 additions & 2 deletions ...-storage-plugin/src/components/object-data-reduction-card/object-data-reduction-card.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
@import '~@patternfly/patternfly/sass-utilities/colors';

.nb-object-data-reduction-card__row-status {
align-items: center;
display: flex;
Expand All @@ -22,3 +20,7 @@
.nb-object-data-reduction-card__row-title {
margin-right: 0.5em;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

.nb-object-data-reduction__item-body--loading {
width: 9rem;
}