Skip to content

Conversation

@rawagner
Copy link
Contributor

@rawagner rawagner commented Jun 17, 2019

utilization

@TheRealJon I've extracted Area chart component which is not using prometheus hook so it can be reused by Dashboard too. Same as I did in #1723 for Gauge.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 17, 2019
@openshift-ci-robot
Copy link
Contributor

Hi @rawagner. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@spadgett spadgett added this to the v4.2 milestone Jun 17, 2019
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2019
@rawagner rawagner force-pushed the dashboard-utilization-2 branch 2 times, most recently from a4dbf6f to 30696b4 Compare June 18, 2019 12:35
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2019
@rawagner rawagner force-pushed the dashboard-utilization-2 branch from 30696b4 to b085840 Compare June 18, 2019 12:43
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2019
@rawagner rawagner force-pushed the dashboard-utilization-2 branch 4 times, most recently from 1f50ada to e1858f5 Compare June 27, 2019 11:49
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2019
@rawagner rawagner force-pushed the dashboard-utilization-2 branch from e1858f5 to ba754e6 Compare June 27, 2019 13:36
@rawagner rawagner changed the title WIP: Dashboard Utilization card Dashboard Utilization card Jun 27, 2019
@openshift-ci-robot 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 Jun 27, 2019
@rawagner rawagner force-pushed the dashboard-utilization-2 branch 2 times, most recently from 741a5e5 to a57a8cc Compare July 1, 2019 17:55
@rawagner
Copy link
Contributor Author

rawagner commented Jul 2, 2019

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 2, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe PF-React also provides such media size breakpoints, so we can reuse them?

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 couldnt find those breakpoints anywhere in PF :/

Copy link
Member

@TheRealJon TheRealJon Jul 8, 2019

Choose a reason for hiding this comment

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

They are in the @patternfly/react-tokens package.

CSS Variable React Token Value
--pf-global--breakpoint--xs global_breakpoint_xs 0
--pf-global--breakpoint--sm global_breakpoint_sm 576px
--pf-global--breakpoint--md global_breakpoint_md 768px
--pf-global--breakpoint--lg global_breakpoint_lg 992px
--pf-global--breakpoint--xl global_breakpoint_xl 1200px
--pf-global--breakpoint--2xl global_breakpoint_2xl 1450px

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheRealJon thanks, im using global_breakpoints from PF in latest update

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: cq => pluginQuery

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

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 extract this into public/components/dashboards-page/overview-dashboard/queries.ts as utilizationQueries?

In other words, we should put all queries in one place for easier maintenance and consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I put those into public/components/dashboards-page/overview-dashboard/queries.ts as utilizationQueries

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: unnatural newline, I'd remove it or add one after both const declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@rawagner rawagner force-pushed the dashboard-utilization-2 branch from a57a8cc to c649911 Compare July 2, 2019 14:27
@rawagner
Copy link
Contributor Author

rawagner commented Jul 2, 2019

@vojtechszocs addressed all your comments

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019
@rawagner
Copy link
Contributor Author

rawagner commented Jul 8, 2019

@TheRealJon Thank you! I will take a look at that date formatting in a follow-on

@spadgett
Copy link
Member

spadgett commented Jul 8, 2019

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2019
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at node_modules/@patternfly/react-tokens/dist/js/index.js the exported objects looks like

{"name":"--pf-global--breakpoint--lg","value":"992px","var":"var(--pf-global--breakpoint--lg)"}

so if value changes its CSS unit from px to something else our layout will probably break.. (not sure we can do anything to prevent that)

Copy link
Member

Choose a reason for hiding this comment

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

I was looking at this too. One (small) problem I have with using these breakpoint vars is that we are using breakpoints against the width of an element (instead of the viewport). Seems a bit arbitrary, and maybe a bit confusing since it will cause reflow at different viewport widths than everything else. I don't think it's a huge deal, which is why I didn't leave a comment originally, but since it came up 😆

@vojtechszocs
Copy link
Contributor

/lgtm

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

12 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@rawagner rawagner force-pushed the dashboard-utilization-2 branch from 37837b6 to ac0d0e7 Compare July 9, 2019 06:47
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2019
@atiratree
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, spadgett, suomiy, TheRealJon, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 4b08faa into openshift:master Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants