Skip to content

Add Dashboard Activity card - #2543

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
rawagner:dashboard-activity
Oct 1, 2019
Merged

Add Dashboard Activity card#2543
openshift-merge-robot merged 1 commit into
openshift:masterfrom
rawagner:dashboard-activity

Conversation

@rawagner

@rawagner rawagner commented Aug 30, 2019

Copy link
Copy Markdown
Contributor

activity

activity-expanded

the Recent section is just a basic event list.

@openshift-ci-robot 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 Aug 30, 2019
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. component/core Related to console core functionality component/sdk Related to console-plugin-sdk labels Aug 30, 2019
@rawagner

Copy link
Copy Markdown
Contributor Author

targeting 4.3

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2019
@rawagner
rawagner force-pushed the dashboard-activity branch from 24a87af to 1561764 Compare August 30, 2019 14:14
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2019
@rawagner
rawagner force-pushed the dashboard-activity branch 2 times, most recently from 5b395a1 to 3fd0670 Compare August 30, 2019 14:30
@andybraren

Copy link
Copy Markdown
Contributor

Oh wow, awesome! 🎉

I'm working on some adjustments to the Recent Events area right now and will comment here when those are ready. Until then, just some minor things I notice:

  • Timestamps within Recent should have 24px of space to the left-hand side, aligning with the "Recent" title
  • We're thinking timestamps would be written like "7:59 pm" to align with the date/time formatting of the rest of the Console - would you agree?
  • Ongoing activities without progress bars (first screenshot) should have some space between the icon and text (4px)
  • Looks like some spacing is needed above the ongoing activity in the second screenshot, matching the space below

@andybraren

andybraren commented Aug 30, 2019

Copy link
Copy Markdown
Contributor

This is still a little WIP (we're targeting to have all cards "finalized" early next week) but here's the latest thinking. Ignore the height of the card for now.

Each event now includes its resource badge when collapsed, making the card as a whole a little more useful without needing to read anything.

When expanded, the resource badge and description within the row disappear, but the timestamp (and error icon) stays. The second timestamp in the expanded area (like "a minute ago" or "Aug 30, 5:55 pm") is no longer needed. Events are typically within the last few hours anyway, and we already have a timestamp with the hour/minute.

The last example just shows the various Ongoing states, which are unchanged. I remember you pointed out that the start time won't always be available, which is fine - that line would just be removed. 🙂

activity-card

@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2019
@rawagner

rawagner commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

Thanks Andy!

Oh wow, awesome!

I'm working on some adjustments to the Recent Events area right now and will comment here when those are ready. Until then, just some minor things I notice:

  • Timestamps within Recent should have 24px of space to the left-hand side, aligning with the "Recent" title

sure, just an oversight :) fixed

  • We're thinking timestamps would be written like "7:59 pm" to align with the date/time formatting of the rest of the Console - would you agree?

I agree that we need to keep formatting aligned, however right now there are actually both formatting used. I see that AM/PM is used for events and 24hrs time is used in charts. So we need to unify it.

  • Ongoing activities without progress bars (first screenshot) should have some space between the icon and text (4px)

Agreed, but since its just a demo activity I didnt really style it :)

  • Looks like some spacing is needed above the ongoing activity in the second screenshot, matching the space below

the space is there, its just scrolled a bit so it looks weird

@rawagner

rawagner commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

This is still a little WIP (we're targeting to have all cards "finalized" early next week) but here's the latest thinking. Ignore the height of the card for now.

Each event now includes its resource badge when collapsed, making the card as a whole a little more useful without needing to read anything.

When expanded, the resource badge and description within the row disappear, but the timestamp (and error icon) stays. The second timestamp in the expanded area (like "a minute ago" or "Aug 30, 5:55 pm") is no longer needed. Events are typically within the last few hours anyway, and we already have a timestamp with the hour/minute.

The last example just shows the various Ongoing states, which are unchanged. I remember you pointed out that the start time won't always be available, which is fine - that line would just be removed.

activity-card

PR updated to latest design

@rawagner rawagner closed this Sep 2, 2019
@rawagner rawagner reopened this Sep 2, 2019
@rawagner

rawagner commented Sep 2, 2019

Copy link
Copy Markdown
Contributor Author

some screenshots of updated recent events section
act_error
act_ok

@rawagner
rawagner force-pushed the dashboard-activity branch 4 times, most recently from 3052285 to 868d0c5 Compare September 3, 2019 12:35
@rawagner
rawagner force-pushed the dashboard-activity branch 3 times, most recently from 13d52de to 73cd976 Compare September 4, 2019 08:48
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 26, 2019
@rawagner
rawagner changed the base branch from master-4.3 to master September 26, 2019 14:27
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 26, 2019
return a.properties.isActivity(queryResults);
})
.map(a => {
const queryResults = a.properties.queries.map(q => prometheusResults.getIn([q, 'data']) as PrometheusResponse);

@afreen23 afreen23 Sep 30, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As per the recent redux state changes, dont we need to get the loadError here as well ?
Otherwise we might need to repoll the queries for the prometheus activities.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Okay I see its implemented

@rawagner
rawagner force-pushed the dashboard-activity branch 13 times, most recently from c670756 to 38991de Compare September 30, 2019 20:12

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's follow the convention used by DevTools plugin, and hopefully other packages will follow up as well:

  • files containing React component(s) reflect the primary component name, e.g. DefaultPage.tsx
  • primary React component is provided via default export

cc @christianvogt @spadgett

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: extra newline.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For now, I'd suggest using a single webpackChunkName value here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For now, I'd suggest using a single webpackChunkName value in all extensions, e.g. demo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
export interface DashboardsOverviewActivity extends DashboardExtension {
export interface DashboardsOverviewResourceActivity extends DashboardExtension {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
type: 'Dashboards/Overview/Prometheus/Activity';
type: 'Dashboards/Overview/Activity/Prometheus';

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
activities: {
resourceActivities: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const activities = getActivities(flags);
const resourceActivities = getActivities(flags);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const allActivities = _.flatten(
const allResourceActivities = _.flatten(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
const getActivities = (flags: FlagsObject) =>
const getResourceActivities = (flags: FlagsObject) =>

@rawagner
rawagner force-pushed the dashboard-activity branch from 590308e to d503234 Compare October 1, 2019 14:10
@vojtechszocs

Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2019
@openshift-ci-robot

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, 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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 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. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/dev-console Related to dev-console component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/monitoring Related to monitoring component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk lgtm Indicates that a PR is ready to be merged. 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.

7 participants