Add Status component to @console/shared #1697

jtomasek · 2019-06-12T11:17:23Z

Introduces common Status components which are supposed to replace current
StatusIconAndText component usage
Expands Icon + Text implementation with Link and Popover capabilities
Allows to define custom resource specific status handling while reusing existing status components (important for plugins)
Allows to easily define custom status components while reusing underlining components provided
Current StatusIconAndText usage is replaces by Status component
Uses PatternFly4 icons

Next steps:

Replace current usage of original StatusIconAndText component with Status component
remove original StatusIconAndText component

christianvogt

Overall I'm not sure of the purpose of these components.
I expect shared components to solve common use cases all console contributions have.

Please elaborate your description.

christianvogt · 2019-06-12T11:59:46Z

frontend/packages/console-shared/src/components/status/status.tsx

+  children: React.ReactNode;
+};
+
+export const Status = ({ icon, children }: StatusProps) => (


Use React.FC<...> to type components.

When I use React.FC, current linting setup forces me to explicitly add type for props which introduces unnecessary duplication:

export const Status: React.FC<StatusProps> = ({ icon, children }: StatusProps) => (...

That's not right. I am currently updating dev console in the monorepo and do not encounter this. Could you provide the error please and indicate what typescript version you are using (hopefully the one from the workspace).

This is the error I am getting: #1539 (review)

IIUC this is addressed by #1721 (see ESLint update commit).

christianvogt · 2019-06-12T12:03:25Z

frontend/packages/console-shared/src/components/status/status.tsx

+
+export const Status = ({ icon, children }: StatusProps) => (
+  <>
+    {icon && <Icon type="pf" name={icon} className="kubevirt-status__icon" />} {children}


Should the space use {' '} to ensure it is rendered? But then you need to do more logic to only include the space of the icon is rendered.

Not exactly sure if the purpose but is it supposed to allow any icon as status?

Originally the space was not included and margin styling was used to ensure the space is present. I'd prefer using space as it adjusts to the font size. I'll fix this to include the space only when icon is present.

christianvogt · 2019-06-12T12:04:22Z

frontend/packages/console-shared/src/components/status/status.tsx

+
+export const PopoverStatus = ({ icon, header, children }: PopoverStatusProps) => (
+  <Popover position="right" headerContent={header} bodyContent={children}>
+    <span className="kubevirt-status__popover">


We should not use non console classnames in shared components.

Yep, I forgot to remove/change this.

christianvogt · 2019-06-12T12:05:46Z

frontend/packages/console-shared/src/components/status/status.tsx

+  <Popover position="right" headerContent={header} bodyContent={children}>
+    <span className="kubevirt-status__popover">
+      <Status icon={icon}>
+        <Button className="kubevirt-status__button" bsStyle="link">


A button without any event handling?

The Button is internally enhanced by Popover and acts as a toggle (https://patternfly-react.surge.sh/patternfly-4/components/popover/)

oh pf... :/
It looks like it will add the event handlers to the span.

christianvogt · 2019-06-12T12:07:11Z

frontend/packages/console-shared/src/components/status/status.tsx

+);
+
+type LinkStatusProps = StatusProps & {
+  linkMessage?: string;


Rename to linkTitle to be consistent with usage

christianvogt · 2019-06-12T12:08:36Z

frontend/packages/console-shared/src/components/status/__tests__/status.test.tsx

+describe('<Status />', () => {
+  it('renders correctly', () => {
+    const component = shallow(testStatus());
+    expect(component).toMatchSnapshot();


The snapshot seems to have a lot of noise.
I also see little value in this test case.

I think this relates to the common practice where each React component has (at least) a simple snapshot test to ensure the rendered markup doesn't change unexpectedly.

As for the messy snapshot file, there are things which don't make sense to serialize (like symbols), meaning we should update Jest & Enzyme & related dependency versions and ensure the generated snapshots are meaningful.

Console doesn't currently have any snapshot tests (git grep 'toMatchSnapshot' returns nothing) so we should revisit the Enzyme (to-JSON) serializer stuff here. This also relates to #1648 which I'm revisiting at the moment.

christianvogt · 2019-06-12T16:04:24Z

@spadgett @vojtechszocs
How do you want to treat the shared package. Should we define some criteria?
Anything generic or anything that is used across multiple packages?

christianvogt · 2019-06-12T16:05:25Z

frontend/packages/console-shared/src/components/status/status.tsx

+
+export const Status = ({ icon, children }: StatusProps) => (
+  <>
+    {icon && <Icon type="pf" name={icon} className="kubevirt-status__icon" />} {children}


Is this suppose to be limited to certain "status" icons? or can they be anything?
PF4 icons don't work this way as they are component based.

Icon comes from patternfly-react package which means PF3 (component showcase).

Supported type includes pf (PatternFly specific) and fa (FontAwesome complementary). name depends on type.

In this case, since type is hardcoded to pf, name could be checked against a list of available PF icon names, but I'm not sure if such list exists.

vojtechszocs · 2019-06-14T15:37:22Z

@spadgett @vojtechszocs
How do you want to treat the shared package. Should we define some criteria?
Anything generic or anything that is used across multiple packages?

Good question. I see that console-shared has constants and selectors used currently by metal3-plugin only. I'd expect other plugin packages to eventually use these as well.

My understanding is that console-shared should contain "anything that is used across multiple packages". (KubeVirt, Metal3 and other plugins are still in their early stages and their code will grow and improve over time.)

vojtechszocs · 2019-06-14T15:49:41Z

frontend/packages/console-shared/src/components/status/fixtures/status.fixture.tsx

+
+import { Status, PopoverStatus } from '../status';
+
+export default [


@jtomasek If I understand correctly, this is the Cosmos fixture format, right?

@christianvogt This could be another use case for console-scripts package - add Cosmos dependency & configuration and use it in packages that wish to leverage this tool.

jtomasek · 2019-06-21T07:12:18Z

/test e2e-aws-console-olm

spadgett · 2019-06-21T13:14:46Z

Next step: Replace current usage of original StatusIconAndText component with GenericStatus component, remove original StatusIconAndText component

This seems like the wrong way to approach this. There are two parts here:

Moving the status icon components into @console/shared.
Updating them with new capabilities.

This looks like you copied status-icon.tsx and made some changes. The code will drift over time if we do this, and it might be hard to get rid of the old component. We're still trying to remove list.tsx as an example.

I think (1) and (2) should be separate PRs, and we shouldn't introduce duplicate components.

/hold

spadgett · 2019-06-21T13:25:20Z

Uses PatternFly4 icons

What icons are you changing? It's hard to tell because you've copied the code to another file, so there's no diff :)

We just recently updated all the icons for this design:

https://openshift.github.io/openshift-origin-design/web-console/4.0-designs/status/status

There were some specific reasons we chose the ones we did.

jtomasek · 2019-06-21T13:52:07Z

Uses PatternFly4 icons

What icons are you changing? It's hard to tell because you've copied the code to another file, so there's no diff :)

I am not changing any icons, I just used PF4 component based counterparts. https://patternfly-react.surge.sh/patternfly-4/icons/Icons/

christianvogt · 2019-06-21T16:06:00Z

frontend/packages/console-shared/src/components/status/generic-status.tsx

+  status?: string;
+};
+
+export const GenericStatus: React.FC<{ status?: string }> = ({ status, children }) => {


I think GenericStatus is a bad name.
React.FC<{ status?: string }> should reference the prop type

christianvogt · 2019-06-21T16:13:05Z

frontend/packages/console-shared/src/components/status/status.tsx

+    <StatusIconAndText icon={icon} title={title} />
+  );
+
+export const SuccessStatus: React.FC<{ title: string }> = ({ title }) => (


Use a pre-defined type for the react props.

christianvogt · 2019-06-21T16:20:48Z

frontend/packages/console-shared/src/components/status/status.tsx

+};
+
+export const StatusIconAndText: React.FC<StatusProps> = ({ icon, title, spin }) => {
+  if (!title) {


If there is an icon but no title, you don't want to show the icon?

I don't think we want to show icon when there is no title. I am sticking with the original StatusIconAndText logic: https://github.com/openshift/console/blob/master/frontend/public/components/utils/status-icon.tsx#L72-L74

christianvogt · 2019-06-21T16:22:55Z

frontend/packages/console-shared/src/components/status/status.tsx

+import { DASH } from '../../constants';
+
+type StatusProps = {
+  icon?: React.ReactElement;


Is the intention to only support patternfly react icons? Or any component? If it's only a patternfly react icon, we can change the prop types to accept color and size just like the pattern fly react icons, and instead of ReactElement accept a component type. Then you can render it without the need to cloneElement.

christianvogt · 2019-06-21T16:26:17Z

@spadgett (fyi @vojtechszocs) for shared components (and any new code in packages in general) do we want to adopt PascalCase component naming along with expecting a single default component export and multiple files, over single large file with multiple components? It's a fairly wide adopted convention and one that patternfly also follows.

jtomasek · 2019-06-24T13:15:52Z

do we want to adopt PascalCase component naming along with expecting a single default component export and multiple files, over single large file with multiple components?

+1

andybraren · 2019-06-24T14:32:25Z

Expands Icon + Text implementation with Link and Popover capabilities

Allows to define custom resource specific status handling while reusing existing status components (important for plugins)

Just +1'ing these from a UX perspective. Status Popovers, documented here in the design repo, are a particularly important part of the UX for the new objects introduced by KubeVirt, Metal3, and some Storage objects as well.

It sounds like the technical approach here needs some tweaking, but UXD agrees that more flexibility around how Statuses can be labeled and interacted with (in both Tables and Resource Details pages) would be a great improvement.

christianvogt · 2019-06-24T18:40:14Z

frontend/public/components/operator-lifecycle-manager/descriptors/status/index.tsx

@@ -79,7 +80,7 @@ export const StatusDescriptor: React.SFC<DescriptorProps> = (props) => {
    </div>
    <dd className="olm-descriptor__value">
      {descriptor.displayName === 'Status' ?
-        (<StatusIconAndText status={value} iconName={value === 'Running' ? 'ok' : undefined} />) :
+        ({value === 'Running' ? <SuccessStatus status={value} /> : <Status status={value} />) :


Just some thoughts.

I'd like to be able to use the same Status component here instead. But we should then enumerate on a set of accepted status values in the prop types:
<Status status={value === 'Running' && 'Complete' || value} />

Alternatively, provide bool props (for the pre-defined finite set of concrete statuses) that take precedence over the status:
<Status status={value} success={value === 'Running'} />

Just some thoughts.

I'd like to be able to use the same Status component here instead. But we should then enumerate on a set of accepted status values in the prop types:
<Status status={value === 'Running' && 'Complete' || value} />

I actually did this initially when I created this patch. But then I realised it is not possible as the status passed as prop is used as a title for the StatusIconAndText so your example value === 'Running' results in Icon + 'Complete' instead of Icon + 'Running'

Alternatively, provide bool props (for the pre-defined finite set of concrete statuses) that take precedence over the status:
<Status status={value} success={value === 'Running'} />

I feel like expanding Status component to address this may be a bit overkill. What we need is to provide a way to define mapping of a certain (custom) status string to predefined status component (which ensures to render correct icon)

e.g.:

switch (status) { case 'discovered': return <SuccessStatus title={status} />; case 'discovering': return <ProgressStatus title={status} />; case 'discovery error': return <ErrorStatus title={status}>{errorMessage}</ErrorStatus>; default: return <Status status={status} /> // fall back to Status component for other states }

This IMHO nicely says: "discovered is a success status"...

Comparing this to current solution which says: "Make 'Running' status act the same as 'Complete' status"

There is currently also one other way to define custom status existing implementation and that is by using iconName on the StatusIconAndText component. (see https://github.com/openshift/console/pull/1774/files#diff-2b92d7e552b962318adc922d3e6f4b61R39). I find this also quite bad as it forces you to specify icon name, which can lead to icon usage inconsistency and problems when icons are refactored/changed.

@spadgett noted a problem of ensuring to achieve consistency of same icon always being used for given status string. I think my changes don't make the current situation worse in this regard. Your example shows, there are cases when it is necessary to deviate from this. My changes add standardised status components which should help avoid this.

heh of course i wasn't thinking about title being the status itself ... argh

vojtechszocs · 2019-06-25T15:47:33Z

frontend/packages/console-shared/src/components/status/status-components.tsx

+
+export const StatusIconAndText: React.FC<StatusIconAndTextProps> = ({ icon, title, spin }) => {
+  if (!title) {
+    return <>{DASH}</>;


Nit: isn't it possible to simply return a string here? (without wrapping it in a fragment, which doesn't contribute any markup on its own)

Unfortunately not! Typescript bug ;/

vojtechszocs · 2019-06-25T16:07:33Z

frontend/packages/console-shared/src/components/status/status.tsx

+};
+
+export const Status: React.FC<StatusProps> = ({ status, children }) => {
+  switch (status) {


It seems that the concepts of status and title are merged together at the moment, carried over from the existing StatusIconAndText component design.

With this Status component, we could map states (statuses?) to titles if we ever decide to decouple these concepts. In general, the status string should be the key indicator to determine icon and any other data needed to render the underlying StatusIconAndText.

vojtechszocs · 2019-06-25T16:11:53Z

frontend/packages/console-shared/src/components/status/status.tsx

+    case 'Completed':
+    case 'Enabled':
+    case 'Ready':
+    case 'Up to date':


Nit: this is inconsistent with other status values where words are always capitalized, but I guess we want to keep it this way for now.

vojtechszocs · 2019-06-25T16:16:40Z

frontend/packages/console-shared/src/components/status/status-components.tsx

+  return (
+    <>
+      {icon &&
+        React.cloneElement(icon, {


Nit: I think we should avoid React.cloneElement if possible.

Did you consider using an Icon prop that references a React component type, instead of its instance?

For example: Icon && <Icon className={...} otherProp={...} />

@christianvogt suggested the same, that solution prevents ability to pass custom props to the icon. We would have to introduce additional iconProps prop to satisfy that. Maybe passing additional props is YAGNI but I would rather keep that option available to provide a way to pass in custom className or id which IMHO can be a valid use case.

vojtechszocs · 2019-06-25T16:19:05Z

do we want to adopt PascalCase component naming along with expecting a single default component export and multiple files, over single large file with multiple components?

+1

+1 from me as well.

vojtechszocs · 2019-06-25T16:24:00Z

adopt PascalCase component naming along with expecting a single default component export and multiple files

If we could use ESLint to check this (in a way that fits the modular eslint-plugin-console "build your own config out of X presets" approach), that'd be great. Any package could opt-into that convention and enforce it via the linter workflow.

spadgett · 2019-06-25T17:23:53Z

Just +1'ing these from a UX perspective. Status Popovers, documented here in the design repo, are a particularly important part of the UX for the new objects introduced by KubeVirt, Metal3, and some Storage objects as well.

But we should look to add this sort of popover consistently throughout console instead of just these specific plugins. My concern is that we're only adding it in a few places and not looking more broadly at existing console pages. Notably we should align the project workloads tab error reporting with these status popovers. They have a similar purpose, but are implemented differently (tooltips). But we also show status in many other places (pods, builds, routes, etc.).

cshinn · 2019-06-25T20:27:46Z

Just +1'ing these from a UX perspective. Status Popovers, documented here in the design repo, are a particularly important part of the UX for the new objects introduced by KubeVirt, Metal3, and some Storage objects as well.

But we should look to add this sort of popover consistently throughout console instead of just these specific plugins. My concern is that we're only adding it in a few places and not looking more broadly at existing console pages. Notably we should align the project workloads tab error reporting with these status popovers. They have a similar purpose, but are implemented differently (tooltips). But we also show status in many other places (pods, builds, routes, etc.).

@spadgett Is it worth making a push to quickly find other places in the UI where this info would be helpful so that the addition isn't too jarring? If we were able to provide locations and content, how hard would it be to add to more pages?

jtomasek · 2019-06-26T11:51:17Z

@spadgett Is it worth making a push to quickly find other places in the UI where this info would be helpful so that the addition isn't too jarring? If we were able to provide locations and content, how hard would it be to add to more pages?

As part of this PR I am replacing all occurrences of existing StatusIconAndText and replacing it with new Status component (second commit 099db40). I think we can easily go through these replacements and identify if there is an error message available and add the message to <Status> component as children, so it gets automatically included in popover.

I found out that there are 2 places where StatusIcon is used as a standalone, so I'll update this PR to allow to cover this use case as well. Then we can completely remove status-icon.tsx to make the transition complete.

jtomasek · 2019-06-28T13:16:36Z

added optional title prop to Status component to allow specify title explicitly, if not provided, status string is used
added optional iconOnly prop to allow rendering status icon only without text
refactored status components into separate modules, PascalCase names, default exports
replaced all usage of original StatusIconAndText and StatusIcon components
removed status-icon.tsx

jtomasek · 2019-07-01T12:16:01Z

/retest

jtomasek · 2019-07-17T09:57:34Z

/test e2e-aws

vojtechszocs · 2019-07-17T15:57:23Z

/lgtm

spadgett · 2019-07-17T16:26:31Z

/hold cancel

jelkosz · 2019-07-17T18:04:07Z

/test e2e-aws-console

jtomasek · 2019-07-17T18:17:39Z

/test e2e-aws-console

openshift-bot · 2019-07-17T20:02:04Z

/retest

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

openshift-bot · 2019-07-17T21:45:54Z

/retest

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

openshift-bot · 2019-07-17T22:11:52Z

/retest

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

openshift-bot · 2019-07-18T00:08:51Z

/retest

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

openshift-bot · 2019-07-18T00:34:50Z

/retest

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

openshift-bot · 2019-07-18T00:47:51Z

/retest

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

openshift-bot · 2019-07-18T01:40:06Z

/retest

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

* Introduces common Status components which are supposed to replace current StatusIconAndText component usage * Expands Icon + Text implementation with Link and Popover capabilities * Allows to define custom resource specific status handling while reusing existing status components (important for plugins) * Allows to easily define custom status components while reusing underlining components provided * Current StatusIconAndText usage is replaces by Status component * Uses PatternFly4 icons * Adds ability to render icon only * Adds ability to specify custom title for the status

Module is replaced with new components in @console/shared/components/status

jtomasek · 2019-07-18T05:56:01Z

Rebased, fixed conflict in packages/metal3-plugin/src/components/host-status.tsx

jtomasek · 2019-07-18T08:02:51Z

/test e2e-aws-console-olm
/test e2e-aws

honza · 2019-07-18T10:35:46Z

/lgtm

vojtechszocs · 2019-07-18T12:33:25Z

/lgtm

openshift-ci-robot · 2019-07-18T12:33:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: honza, jtomasek, vojtechszocs

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

The pull request process is described here

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 added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 12, 2019

openshift-ci-robot requested review from dtaylor113 and jeff-phillips-18 June 12, 2019 11:17

christianvogt reviewed Jun 12, 2019

View reviewed changes

vojtechszocs reviewed Jun 14, 2019

View reviewed changes

jtomasek mentioned this pull request Jun 20, 2019

Add host status column #1774

Merged

jtomasek mentioned this pull request Jun 21, 2019

Add host list filters #1786

Merged

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

christianvogt reviewed Jun 21, 2019

View reviewed changes

christianvogt reviewed Jun 24, 2019

View reviewed changes

vojtechszocs reviewed Jun 25, 2019

View reviewed changes

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 1, 2019

openshift-ci-robot assigned vojtechszocs Jul 17, 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 Jul 17, 2019

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

Jiri Tomasek added 3 commits July 18, 2019 07:27

Replace StatusIconAndText with Status component

f0b89df

Delete unused status-icon module

a87bf7e

Module is replaced with new components in @console/shared/components/status

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 18, 2019

openshift-ci-robot assigned honza Jul 18, 2019

openshift-merge-robot merged commit badfbbf into openshift:master Jul 18, 2019

spadgett added this to the v4.2 milestone Jul 19, 2019

rawagner mentioned this pull request Jul 22, 2019

Add Baremetal hosts to overview dashboard inventory #2069

Merged


		import { Status, PopoverStatus } from '../status';

		export default [

Add Status component to @console/shared #1697

Add Status component to @console/shared #1697

Conversation

jtomasek commented Jun 12, 2019 • edited Loading

christianvogt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Jun 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs commented Jun 14, 2019

Choose a reason for hiding this comment

jtomasek commented Jun 21, 2019

spadgett commented Jun 21, 2019

spadgett commented Jun 21, 2019

jtomasek commented Jun 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Jun 21, 2019

jtomasek commented Jun 24, 2019 • edited Loading

andybraren commented Jun 24, 2019

Choose a reason for hiding this comment

jtomasek Jun 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs commented Jun 25, 2019

vojtechszocs commented Jun 25, 2019

spadgett commented Jun 25, 2019

cshinn commented Jun 25, 2019

jtomasek commented Jun 26, 2019 • edited Loading

jtomasek commented Jun 28, 2019

jtomasek commented Jul 1, 2019

jtomasek commented Jul 17, 2019

vojtechszocs commented Jul 17, 2019

spadgett commented Jul 17, 2019

jelkosz commented Jul 17, 2019

jtomasek commented Jul 17, 2019

openshift-bot commented Jul 17, 2019

openshift-bot commented Jul 17, 2019

openshift-bot commented Jul 17, 2019

openshift-bot commented Jul 18, 2019

openshift-bot commented Jul 18, 2019

openshift-bot commented Jul 18, 2019

openshift-bot commented Jul 18, 2019

jtomasek commented Jul 18, 2019

jtomasek commented Jul 18, 2019

honza commented Jul 18, 2019

vojtechszocs commented Jul 18, 2019

openshift-ci-robot commented Jul 18, 2019

jtomasek commented Jun 12, 2019 •

edited

Loading

christianvogt Jun 12, 2019 •

edited

Loading

jtomasek commented Jun 24, 2019 •

edited

Loading

jtomasek Jun 25, 2019 •

edited

Loading

jtomasek commented Jun 26, 2019 •

edited

Loading