Update Inventory card design #2601

rawagner · 2019-09-05T12:06:01Z

every item shows maximum of 3 most important statuses
item title is link to resource page
use skeleton loading

rawagner · 2019-09-05T12:07:17Z

/hold

andybraren · 2019-09-05T14:25:19Z

Nice, looks great! 🎉

rawagner · 2019-09-17T10:20:07Z

Added ability to expand inventory item

mareklibra · 2019-09-17T12:38:02Z

lgtm, blocked on missing 4.3 branch

vojtechszocs · 2019-09-27T14:45:43Z

frontend/packages/console-demo-plugin/src/dashboards/inventory.tsx

@@ -13,3 +16,7 @@ export const getRouteStatusGroups: StatusGroupMapper = (resources) => ({
 export const DemoGroupIcon: React.FC<{}> = () => (
  <AddressBookIcon className="co-inventory-card__status-icon co-inventory-card__status-icon--warn" />
 );
+
+export const ExpandedRoutes: React.FC<ExpandedComponentProps> = ({ resource }) => (
+  <div>additional content for {resource.length} routes</div>


Nit: shouldn't we capitalize the word "additional" ?

Capitalized :)

vojtechszocs · 2019-09-27T14:48:37Z

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

@@ -118,6 +121,8 @@ namespace ExtensionProperties {

    /** Function which will map various statuses to groups. */
    mapper: StatusGroupMapper;
+
+    expandedComponentLoader?: LazyLoader<ExpandedComponentProps>;


Please avoid the Loader suffix.

The way how React components are referenced from extension's properties object shouldn't affect the property name. Let's keep things short and simple.

ok, removed

vojtechszocs · 2019-09-27T14:50:01Z

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

@@ -126,6 +131,9 @@ namespace ExtensionProperties {

    /** React component representing status group icon. */
    icon: React.ReactElement;
+
+    /** Priority of inventory group */


Please document the practical impact of this property, e.g. groups with higher prio are rendered before groups with lower prio. This is especially important in case of non-optional extension properties.

Also, does it make sense to have priority optional here? We should keep the mandatory set of extension properties small and use reasonable defaults whenever possible.

Ok, i guess this is not really needed. I removed the whole priority thing.
The ordering of statuses are as follows:
error, warning, progress, ...whatever is defined by plugin, unknown

vojtechszocs · 2019-09-27T14:55:34Z

frontend/public/components/dashboard/inventory-card/inventory-card.scss

  display: flex;
  font-size: 1rem;
  justify-content: space-between;
-  padding: 1em var(--pf-c-card--child--PaddingRight) 1em var(--pf-c-card--child--PaddingLeft);
+  line-height: 1.5rem;
+  padding: 1rem var(--pf-c-card--child--PaddingRight) 1rem var(--pf-c-card--child--PaddingLeft);


Just curious, the var(x) expression is the standard CSS way of handling variables, equivalent to Sass $x expressions? If so, we should favor the new CSS way to handle variables, right?

Also, what is the rationale behind var(--pf-x) instead of non-prefixed var(pf-x) ?

I couldnt find variable in scss form that I could use. Also patternfly-react is using these in same way.

OK, so I guess $pf-stuff comes from PF3 (Sass) and var(--pf-stuff) comes from PF4, is that right?

Yes, correct

vojtechszocs · 2019-09-27T14:58:04Z

frontend/public/components/dashboard/inventory-card/inventory-card.scss

+  &::after {
+    background-repeat: no-repeat;
+    content:"";
+    display:block;


Nit: missing space after : in lines above, and possibly on other places too.

Since we don't use Stylelint (yet) we can't check this in an automated way.

vojtechszocs · 2019-09-27T15:13:37Z

frontend/public/components/dashboard/inventory-card/inventory-item.tsx

+  flags = {},
+  ExpandedComponent,
+}) => {
+  const TitleComponent = React.useCallback((props) => (


This is interesting, why React.useCallback and not React.memo here?

Is it because of explicit dependency control declared through array [kind, namespace] ?

I can define dependencies for React.memo too. But React.memo returns value of memoized function, React.useCallback returns memoized function (not its value)

Great, thanks for explaining this.

vojtechszocs · 2019-09-27T15:15:48Z

frontend/public/components/dashboard/inventory-card/inventory-item.tsx

+export type StatusGroupMapper = (
+  resources: K8sResourceKind[],
+  additionalResources?: {[key: string]: K8sResourceKind[]},
+) => {[key in InventoryStatusGroup | string]: {filterType?: string, statusIDs: string[], count: number}};


Nit: please extract return type into separate type declaration.

vojtechszocs · 2019-09-27T15:17:23Z

frontend/public/components/dashboard/inventory-card/inventory-item.tsx

  count: number;
  children?: React.ReactNode;
  error: boolean;
+  TitleComponent?: React.ComponentType<any>;
+  ExpandedComponent?: React.ComponentType<any>;


Nit: ComponentType<P = {}> has fallback to {} so do we really need an explicit any type parameter?

I think that {} is a better (more sensible) type fallback than any when it comes to React component props.

ok, i wasnt sure which is better..using ComponentType<{}>

vojtechszocs · 2019-09-27T15:20:46Z

frontend/public/components/dashboard/inventory-card/status-group.ts

 }
+
+export const inventoryStatusGroupPriority = {


When specifying priority value in the plugin, can we import this and do something like

inventoryStatusGroupPriority[InventoryStatusGroup.PROGRESS] + some_offset

instead of hard-coded numbers everywhere? This also creates a link to base priority values and therefore improves the understanding of extension's impact.

sounds good, using offest values now

since I removed at at the end..this is irrelevant now

vojtechszocs · 2019-09-27T15:22:56Z

frontend/public/components/dashboards-page/overview-dashboard/inventory-card.tsx

-  const pvcsLoadError = _.get(resources.pvcs, 'loadError');
-  const pvcsData = _.get(resources.pvcs, 'data', []) as K8sResourceKind[];
+  const ExpandedComponent = React.useCallback(() =>
+    <AsyncComponent loader={expandedComponentLoader} resource={resourceData} additionalResources={additionalResourcesData} />


Nit: the line seems too long, please split the JSX expression onto multiple lines.

vojtechszocs · 2019-09-30T13:38:52Z

frontend/packages/console-plugin-sdk/src/typings/dashboards.ts

@@ -118,6 +121,8 @@ namespace ExtensionProperties {

    /** Function which will map various statuses to groups. */
    mapper: StatusGroupMapper;
+
+    expandedComponent?: LazyLoader<ExpandedComponentProps>;


Please add a brief description here.

vojtechszocs · 2019-09-30T13:40:19Z

frontend/public/components/dashboard/inventory-card/inventory-card.scss

  display: flex;
  font-size: 1rem;
  justify-content: space-between;
-  padding: 1em var(--pf-c-card--child--PaddingRight) 1em var(--pf-c-card--child--PaddingLeft);
+  line-height: 1.5rem;
+  padding: 1rem var(--pf-c-card--child--PaddingRight) 1rem var(--pf-c-card--child--PaddingLeft);


OK, so I guess $pf-stuff comes from PF3 (Sass) and var(--pf-stuff) comes from PF4, is that right?

vojtechszocs · 2019-09-30T14:33:52Z

/lgtm
/approve

openshift-bot · 2019-09-30T16:24:45Z

/retest

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

spadgett · 2019-09-30T16:31:05Z

/hold
for #2865 (sorry)

- every item shows maximum of 3 most important statuses - item title is link to resource page - use skeleton loading - item can be expanded

spadgett · 2019-09-30T18:59:22Z

/hold cancel

mareklibra · 2019-10-01T06:47:18Z

/lgtm

openshift-ci-robot · 2019-10-01T06:47:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mareklibra, 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:

~~frontend/OWNERS~~ [vojtechszocs]

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 requested review from honza and jcaianirh September 5, 2019 12:06

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2019

rawagner mentioned this pull request Sep 6, 2019

Rerender only when data for given reduxes has changed #2564

Merged

rawagner force-pushed the 4.3-inventory branch 2 times, most recently from 247309c to 1eac0d1 Compare September 17, 2019 10:19

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 17, 2019

rawagner force-pushed the 4.3-inventory branch from 1eac0d1 to 6fcf25d Compare September 17, 2019 12:14

mareklibra mentioned this pull request Sep 18, 2019

kubevirt: Add VM Dashboard Inventory Card #2751

Merged

2 tasks

rawagner force-pushed the 4.3-inventory branch from 6fcf25d to 4518c7a Compare September 21, 2019 07:03

rawagner changed the base branch from master to master-4.3 September 21, 2019 07:03

openshift-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 21, 2019

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2019

rawagner force-pushed the 4.3-inventory branch 3 times, most recently from 27981b1 to ee7a63d Compare September 26, 2019 14:02

mareklibra mentioned this pull request Sep 27, 2019

Add PodDashboardInventoryCard component #2833

Closed

3 tasks

vojtechszocs suggested changes Sep 27, 2019

View reviewed changes

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

rawagner force-pushed the 4.3-inventory branch 3 times, most recently from f417d56 to 48b3a2f Compare September 30, 2019 08:18

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

vojtechszocs suggested changes Sep 30, 2019

View reviewed changes

rawagner force-pushed the 4.3-inventory branch from e245031 to c519a4e Compare September 30, 2019 14:33

openshift-ci-robot assigned vojtechszocs Sep 30, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 30, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2019

rawagner force-pushed the 4.3-inventory branch from c519a4e to bf479dd Compare September 30, 2019 18:22

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 30, 2019

Update Inventory card design

6e83d03

- every item shows maximum of 3 most important statuses - item title is link to resource page - use skeleton loading - item can be expanded

rawagner force-pushed the 4.3-inventory branch from bf479dd to 6e83d03 Compare September 30, 2019 18:48

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2019

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

openshift-merge-robot merged commit 5310054 into openshift:master Oct 1, 2019

Update Inventory card design #2601

Update Inventory card design #2601

Uh oh!

Conversation

rawagner commented Sep 5, 2019

Uh oh!

rawagner commented Sep 5, 2019

Uh oh!

andybraren commented Sep 5, 2019

Uh oh!

rawagner commented Sep 17, 2019

Uh oh!

mareklibra commented Sep 17, 2019

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!

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!

vojtechszocs commented Sep 30, 2019

Uh oh!

openshift-bot commented Sep 30, 2019

Uh oh!

spadgett commented Sep 30, 2019

Uh oh!

spadgett commented Sep 30, 2019

Uh oh!

mareklibra commented Oct 1, 2019

Uh oh!

openshift-ci-robot commented Oct 1, 2019

Uh oh!