fix(serverless,pipeline): apply release badges to serverless and pipeline pages #2427

nemesis09 · 2019-08-20T18:17:08Z

Fixes Bugs -

This PR -

Adds support for Dev Preview Badge
Applies Dev Preview Badge to pipeline list, pipeline details page and pipeline run details pages
Applies Tech Preview Badge to serverless pages in the admin perspective

Screens -

nemesis09 · 2019-08-21T09:12:46Z

rohitkrai03 · 2019-08-21T15:33:04Z

@nemesis09 The JIRA issue mentions the badge needs to be applied in Serverless resource pages as well. The import flows with serverless checkbox already has the badge so that's done.

christianvogt · 2019-08-21T13:50:47Z

frontend/public/components/utils/headings.tsx

@@ -54,11 +62,15 @@ export const PageHeading = connectToModel((props: PageHeadingProps) => {
  const hasButtonActions = !_.isEmpty(buttonActions);
  const hasMenuActions = _.isFunction(menuActions) || !_.isEmpty(menuActions);
  const showActions = (hasButtonActions || hasMenuActions) && !_.isEmpty(data) && !_.get(data, 'deletionTimestamp');
+  const isTechPreview = props.isTechPreview;


Deconstruct this value on line 50 with the other props.

christianvogt · 2019-08-21T16:35:51Z

frontend/public/components/factory/list-page.jsx

@@ -236,7 +237,7 @@ export const FireMan_ = connect(null, {filterList})(

      const {title} = this.props;
      return <React.Fragment>
-        {title && <PageHeading title={title} />}
+        {title && <PageHeading title={title} isTechPreview={this.props.isTechPreview}/>}


Deconstruct isTechPreview at line 238

christianvogt · 2019-08-21T16:44:28Z

frontend/public/components/utils/headings.tsx

+      </ol>
+    </div>
+    <div className="co-breadcrumbs--tech-preview-badge">
+        <TechPreviewBadge />


You are applying the TechPreviewBadge to all Breadcrumbs which is wrong.
This should be conditional and probably better controlled outside of the Breadcrumbs component and in the heading itself.

This isn't a good layout. Flow layouts will always be more acceptable than absolute positioned elements and less prone to breakages.

You may be better off moving the addition of the tech preview badge from Breadcrumbs to PageHeading and applying a flex layout there with align-items center.

I've added the badge to PageHeading using Split layout.

nemesis09 · 2019-08-21T17:52:09Z

@nemesis09 The JIRA issue mentions the badge needs to be applied in Serverless resource pages as well. The import flows with serverless checkbox already has the badge so that's done.

Yes. The serverless resource pages already have the tech preview badge.

debsmita1 · 2019-08-23T06:07:44Z

/test e2e-aws

nemesis09 · 2019-08-27T15:31:00Z

@nemesis09 The JIRA issue mentions the badge needs to be applied in Serverless resource pages as well. The import flows with serverless checkbox already has the badge so that's done.

Yes. The serverless resource pages already have the tech preview badge.

christianvogt · 2019-08-27T18:23:00Z

frontend/public/components/utils/headings.tsx

@@ -56,9 +62,19 @@ export const PageHeading = connectToModel((props: PageHeadingProps) => {
  const showActions = (hasButtonActions || hasMenuActions) && !_.isEmpty(data) && !_.get(data, 'deletionTimestamp');

  return <div className={classNames('co-m-nav-title', {'co-m-nav-title--detail': detail}, {'co-m-nav-title--logo': isCSV}, {'co-m-nav-title--breadcrumbs': breadcrumbsFor && !_.isEmpty(data)})} style={style}>
-    { breadcrumbsFor && !_.isEmpty(data) && <BreadCrumbs breadcrumbs={breadcrumbsFor(data)} /> }
+    <Split>


Suggested change

<Split>

<Split style={{alignItems: 'baseline'}}>

christianvogt · 2019-08-27T18:23:24Z

frontend/public/components/utils/headings.tsx

+        { breadcrumbsFor && !_.isEmpty(data) && <BreadCrumbs breadcrumbs={breadcrumbsFor(data)} />}
+      </SplitItem>
+      <SplitItem>
+        <div className="co-pipeline-details--tech-preview-badge">


Remove this <div> and the associated class co-pipeline-details--tech-preview-badge and style file.

christianvogt · 2019-08-27T18:29:14Z

frontend/public/components/utils/headings.tsx

@@ -119,6 +135,7 @@ export type PageHeadingProps = {
  title?: string | JSX.Element;
  titleFunc?: (obj: K8sResourceKind) => string | JSX.Element;
  customData?: any;
+  isTechPreview?: boolean;


You should probably look at supporting the new DevPreviewBadge as well now instead of a boolean.
Maybe accept a component named badge and let the parent container provide either a TechPreviewBadge or DevPreviewBadge?

christianvogt · 2019-08-30T19:03:28Z

@nemesis09 any updates on this?

vikram-raj

Isn't pipeline run detail page should also have tech preview badge? @christianvogt @serenamarie125

nemesis09 · 2019-09-03T06:38:48Z

@nemesis09 any updates on this?

Waiting to test a change. other than that, its ready.

nemesis09 · 2019-09-03T16:31:41Z

Isn't pipeline run detail page should also have tech preview badge? @christianvogt @serenamarie125

I guess it should. So I've added it.

christianvogt · 2019-09-03T21:11:37Z

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

+  <Label style={{ backgroundColor: '#D93F00' }}>Dev Preview</Label>
+);
+
+export const ReleaseBadge: React.FC<ReleaseBadgeProps> = ({ releaseBadgeType }) => (


This component wouldn't be necessary if there was no string to bade mapping.

I agree here. This could quickly get messy if we add more badges in future. It's better if parent sends the . component directly instead of sending badge type prop.

christianvogt · 2019-09-03T21:14:02Z

frontend/public/components/utils/headings.tsx

@@ -119,6 +131,7 @@ export type PageHeadingProps = {
  title?: string | JSX.Element;
  titleFunc?: (obj: K8sResourceKind) => string | JSX.Element;
  customData?: any;
+  releaseBadgeType?: string;


If you made this badge: React.ReactNode then the parent could simply supply a custom badge component of either DevPreviewBadge or TechPreviewBadge instead of mapping a string to a badge type in case we later have additional badges or badges that are linked to documentation or some other customization.

@nemesis09 Better just remove the ReleaseBadge component and use individual DevPreviewBadge and TechPreviewComponent components as suggested by @christianvogt .

christianvogt · 2019-09-03T21:14:26Z

frontend/public/components/utils/headings.tsx

+      <SplitItem isFilled>
+        { breadcrumbsFor && !_.isEmpty(data) && <BreadCrumbs breadcrumbs={breadcrumbsFor(data)} />}
+      </SplitItem>
+      <SplitItem>


You can omit this SplitItem as well if there is no badge to display.

christianvogt · 2019-09-03T21:15:23Z

frontend/public/components/command-line-tools.tsx

@@ -50,7 +50,7 @@ const CommandLineTools: React.FC<CommandLineToolsProps> = ({obj}) => {
      </p>
      <hr />
      <h2 className="co-section-heading">odo - Developer-focused CLI for OpenShift</h2>
-      <p><TechPreviewBadge /></p>
+      <p><ReleaseBadge releaseBadgeType="tech-preview" /></p>


No reason to change this from TechPreviewBadge

Yes, and for that you need to export both DevPreviewBadge and TechPreviewBadge in the index file of @console/shared as suggested above.

rohitkrai03 · 2019-09-03T21:21:03Z

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

+  <Label style={{ backgroundColor: '#D93F00' }}>Dev Preview</Label>
+);
+
+export const ReleaseBadge: React.FC<ReleaseBadgeProps> = ({ releaseBadgeType }) => (


I agree here. This could quickly get messy if we add more badges in future. It's better if parent sends the . component directly instead of sending badge type prop.

rohitkrai03 · 2019-09-03T21:24:37Z

frontend/packages/console-shared/src/components/index.ts

@@ -1,3 +1,3 @@
-export { default as TechPreviewBadge } from './TechPreviewBadge';
+export { default as ReleaseBadge } from './ReleaseBadge';


@nemesis09 This change will break the Serverless section in import flows as it imports TechPreviewBadge.

Suggested change

export { default as ReleaseBadge } from './ReleaseBadge';

export * from './ReleaseBadge';

rohitkrai03 · 2019-09-03T21:25:21Z

frontend/public/components/command-line-tools.tsx

@@ -50,7 +50,7 @@ const CommandLineTools: React.FC<CommandLineToolsProps> = ({obj}) => {
      </p>
      <hr />
      <h2 className="co-section-heading">odo - Developer-focused CLI for OpenShift</h2>
-      <p><TechPreviewBadge /></p>
+      <p><ReleaseBadge releaseBadgeType="tech-preview" /></p>


Yes, and for that you need to export both DevPreviewBadge and TechPreviewBadge in the index file of @console/shared as suggested above.

rohitkrai03 · 2019-09-03T21:28:32Z

frontend/public/components/utils/headings.tsx

@@ -119,6 +131,7 @@ export type PageHeadingProps = {
  title?: string | JSX.Element;
  titleFunc?: (obj: K8sResourceKind) => string | JSX.Element;
  customData?: any;
+  releaseBadgeType?: string;


@nemesis09 Better just remove the ReleaseBadge component and use individual DevPreviewBadge and TechPreviewComponent components as suggested by @christianvogt .

nemesis09 · 2019-09-05T10:41:34Z

@christianvogt @rohitkrai03
I've made the necessary changes conforming to the comments.

christianvogt · 2019-09-06T12:55:11Z

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

+import * as React from 'react';
+import { Label } from '@patternfly/react-core';
+
+export const TechPreviewBadge: React.FC = () => (


These should be their own separate components since we want to follow the convention of one Component per file.
If you want to make a sub dir named badges, that's ok.

christianvogt · 2019-09-06T12:57:06Z

frontend/public/components/utils/headings.tsx

-    { breadcrumbsFor && !_.isEmpty(data) && <BreadCrumbs breadcrumbs={breadcrumbsFor(data)} /> }
+    <Split style={{alignItems: 'baseline'}}>
+      <SplitItem isFilled>
+        { breadcrumbsFor && !_.isEmpty(data) && <BreadCrumbs breadcrumbs={breadcrumbsFor(data)} />}


Looks like you should omit the entire <Split> component if breadcrumbsFor is false

Yes. It's fixed now.

nemesis09 · 2019-09-09T15:21:33Z

/retest

christianvogt · 2019-09-09T20:05:05Z

frontend/public/components/utils/headings.tsx

  return <div className={classNames('co-m-nav-title', {'co-m-nav-title--detail': detail}, {'co-m-nav-title--logo': isCSV}, {'co-m-nav-title--breadcrumbs': breadcrumbsFor && !_.isEmpty(data)})} style={style}>
-    { breadcrumbsFor && !_.isEmpty(data) && <BreadCrumbs breadcrumbs={breadcrumbsFor(data)} /> }
+    <Split style={{alignItems: 'baseline'}}>


Only add this when breadcumbsFor is truthy.

…less pages

nemesis09 · 2019-09-10T17:34:17Z

/test e2e-aws-console-olm

christianvogt · 2019-09-10T18:21:49Z

/lgtm
/approve

openshift-ci-robot · 2019-09-10T18:22:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, nemesis09

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~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nemesis09 · 2019-09-10T23:02:07Z

/test e2e-aws-console-olm

openshift-ci-robot requested review from christianvogt and rohitkrai03 August 21, 2019 09:12

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/core Related to console core functionality component/dev-console Related to dev-console labels Aug 21, 2019

christianvogt reviewed Aug 21, 2019

View reviewed changes

nemesis09 force-pushed the fix-tech-preview branch 2 times, most recently from f9eaafb to 98d1b45 Compare August 22, 2019 12:22

nemesis09 force-pushed the fix-tech-preview branch 3 times, most recently from 6e98149 to f9a72e5 Compare August 27, 2019 15:19

nemesis09 closed this Aug 27, 2019

nemesis09 reopened this Aug 27, 2019

christianvogt reviewed Aug 27, 2019

View reviewed changes

vikram-raj reviewed Sep 3, 2019

View reviewed changes

nemesis09 force-pushed the fix-tech-preview branch from f9a72e5 to 5ebff8e Compare September 3, 2019 12:33

openshift-ci-robot added component/knative Related to knative-plugin component/shared Related to console-shared size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 3, 2019

nemesis09 changed the title ~~fix(pipeline): apply tech preview badges to pipeline pages~~ fix(serverless,pipeline): apply release badges to serverless and pipeline pages Sep 3, 2019

nemesis09 force-pushed the fix-tech-preview branch 2 times, most recently from d313da7 to 66dce34 Compare September 3, 2019 16:30

nemesis09 force-pushed the fix-tech-preview branch from 66dce34 to e119ade Compare September 3, 2019 19:43

christianvogt reviewed Sep 3, 2019

View reviewed changes

rohitkrai03 suggested changes Sep 3, 2019

View reviewed changes

nemesis09 force-pushed the fix-tech-preview branch from e119ade to 2ef62b8 Compare September 5, 2019 10:38

nemesis09 force-pushed the fix-tech-preview branch from 2ef62b8 to d1b5521 Compare September 5, 2019 12:41

christianvogt reviewed Sep 6, 2019

View reviewed changes

nemesis09 force-pushed the fix-tech-preview branch from d1b5521 to 5856cb6 Compare September 9, 2019 14:02

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 9, 2019

christianvogt reviewed Sep 9, 2019

View reviewed changes

nemesis09 force-pushed the fix-tech-preview branch from 5856cb6 to ba40aa4 Compare September 10, 2019 06:19

fix(serverless,pipeline): apply release badges to pipeline and server…

1f2f925

…less pages

nemesis09 force-pushed the fix-tech-preview branch from ba40aa4 to 1f2f925 Compare September 10, 2019 14:11

openshift-ci-robot assigned christianvogt Sep 10, 2019

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 10, 2019

spadgett added this to the v4.2 milestone Sep 11, 2019

openshift-merge-robot merged commit adaba90 into openshift:master Sep 11, 2019

nemesis09 deleted the fix-tech-preview branch September 19, 2019 09:33

		@@ -1,3 +1,3 @@
		export { default as TechPreviewBadge } from './TechPreviewBadge';
		export { default as ReleaseBadge } from './ReleaseBadge';

	export { default as ReleaseBadge } from './ReleaseBadge';
	export * from './ReleaseBadge';

fix(serverless,pipeline): apply release badges to serverless and pipeline pages #2427

fix(serverless,pipeline): apply release badges to serverless and pipeline pages #2427

Conversation

nemesis09 commented Aug 20, 2019 • edited

nemesis09 commented Aug 21, 2019

rohitkrai03 commented Aug 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

Choose a reason for hiding this comment

nemesis09 Aug 27, 2019 • edited

Choose a reason for hiding this comment

nemesis09 commented Aug 21, 2019

debsmita1 commented Aug 23, 2019

nemesis09 commented Aug 27, 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 Aug 30, 2019

vikram-raj left a comment

Choose a reason for hiding this comment

nemesis09 commented Sep 3, 2019

nemesis09 commented Sep 3, 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

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

nemesis09 commented Sep 5, 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

nemesis09 commented Sep 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesis09 commented Sep 10, 2019

christianvogt commented Sep 10, 2019

openshift-ci-robot commented Sep 10, 2019

nemesis09 commented Sep 10, 2019

nemesis09 commented Aug 20, 2019 •

edited

nemesis09 Aug 27, 2019 •

edited