Skip to content
This repository has been archived by the owner on Apr 28, 2020. It is now read-only.

Dashboard, Utilization: Render in narrow container #314

Merged
merged 3 commits into from
Apr 8, 2019

Conversation

mareklibra
Copy link
Contributor

The Utilization component renders different sub-component structure
for a narrow container.

@coveralls
Copy link

coveralls commented Apr 5, 2019

Pull Request Test Coverage Report for Build 1285

  • 18 of 22 (81.82%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 87.599%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/Dashboard/Utilization/UtilizationItem.js 18 22 81.82%
Totals Coverage Status
Change from base Build 1056: -0.09%
Covered Lines: 3163
Relevant Lines: 3464

💛 - Coveralls

@mareklibra mareklibra changed the title Dashboard, Utilization: Render in narrow container WIP Dashboard, Utilization: Render in narrow container Apr 5, 2019
@mareklibra
Copy link
Contributor Author

mareklibra commented Apr 5, 2019

narrow

@mareklibra mareklibra changed the title WIP Dashboard, Utilization: Render in narrow container Dashboard, Utilization: Render in narrow container Apr 5, 2019
@rawagner
Copy link
Contributor

rawagner commented Apr 5, 2019

I would expect that the component behaviour would be more dynamic, controlled by actual size of the card, not by isNarrow prop. Maybe we could use some library like react-bounds. @suomiy @mareklibra do you guys have any experience with that ?

@mareklibra
Copy link
Contributor Author

@rawagner , not yet but I can give it a try. Based on documentation, it looks great.

@mareklibra
Copy link
Contributor Author

The use of react-bounds seems to be straightforward, just unit testing is not so simple as without it (requires the component to be mounted).

Let's see it in action for a moment, it might make sense to replace the MediaQuery by this library.

@rawagner
Copy link
Contributor

rawagner commented Apr 5, 2019

@mareklibra looks great! Can you center Loading and Not Available texts ?
utilization_loading

chart = <SparklineChart id={id} data={chartData} axis={axis} unloadBeforeLoad />;
}

if (isBound('narrow')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it would be easier to maintain if we keep only one return statement and parametrize Column sizes and Chart part. For the first div with different className with can use classNames function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the component structure is different, I use two <Row> components in the narrow case

@atiratree
Copy link
Contributor

I know react-responsive is not perfect but I would advise against using react-bounds. Mainly because this project has only one maintainer and is not actively maintained.

I think the desired functionality can be achieved with react-responsive.

@mareklibra
Copy link
Contributor Author

I think the desired functionality can be achieved with react-responsive.

Good point with the maintenance ...
IIUC, react-responsive can not be used for checking container element boundary, just for the window size.

@rawagner
Copy link
Contributor

rawagner commented Apr 8, 2019

how about react-measure ? It seems to be a lot more active than react-bounds

The Utilization component renders different sub-component structure
for a narrow container.
@mareklibra
Copy link
Contributor Author

The react-measure is slightly harder to work with, but its much better unit-testable.
The project is active atm and serves our needs, so this PR is reimplemented to use it.

@rawagner
Copy link
Contributor

rawagner commented Apr 8, 2019

LGTM. @suomiy any objections against react-measure ?

@atiratree
Copy link
Contributor

atiratree commented Apr 8, 2019

react-responsive can not be used for checking container element boundary, just for the window size.

I think it could make more sense to give the graph more space and use only the narrow version only?

Also the x axis + units are missing.

Although react-measure is a bit better (still 1-2 maintainers). Is this really needed? Can't we rather design alongside patternfly breakpoints? This alternative way will morph the page quite often.

@mareklibra
Copy link
Contributor Author

I think it could make more sense to give the graph more space and use only the narrow version only?

I like the wider (recent) design more if space is available.

Also the x axis + units are missing.

I will play with the X-axis independently together with the time-period dropdown selector. I can not use X-axis features of the C3chart for that to meet recent design.

The units will be implemented within this PR, thanks for spotting it.

Can't we rather design alongside patternfly breakpoints?

This gives us wider flexibility. Using media breakpoints will not solve resizing of an inner component based on available space. Recently, the same component will be used either in wide central column or narrow right-column. Without checking boundary on the component-level, this is hard to achieve.

@rawagner
Copy link
Contributor

rawagner commented Apr 8, 2019

X-axis will be solved in a followup issue.

@rawagner rawagner merged commit 2d2dc0d into kubevirt:master Apr 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants