Add QuickStarts cta to create ProjectHelmChartRepository in the helm catalog description text #10904

debsmita1 · 2022-01-21T16:54:13Z

JIRA story:
https://issues.redhat.com/browse/ODC-6466

Root analysis:
As a user, I want to see the QuickStart which will guide me through the process of adding my HelmChartRepo to the desired project.

Solution description:

Modified the catalogDescription property of CatalogItemType extension to also accept JSX Element
showing the quick start link only if the namespaced-hcr quickstart is available

Screenshot:

Test coverage:

TODO:

Add the new quick start name to the link (depends on Add QS for installing namespeced Helm Chart Repos console-operator#631)

debsmita1 · 2022-01-21T16:55:15Z

/kind feature
/assign @rohitkrai03
/cc @serenamarie125

debsmita1 · 2022-01-21T16:56:30Z

/assign @invincibleJai

jerolimov · 2022-01-24T07:56:05Z

frontend/packages/console-shared/src/components/catalog/CatalogController.tsx

@@ -174,7 +190,7 @@ const CatalogController: React.FC<CatalogControllerProps> = ({
      <div className="co-m-page__body">
        <div className="co-catalog">
          <PageHeading title={title} breadcrumbs={type ? breadcrumbs : null} />
-          <p className="co-catalog-page__description">{description}</p>
+          {getCatalogDescription()}


@debsmita1 Just read the code, and all three cases adds <p className="co-catalog-page__description"> around the component they render. Two times in getCatalogDescription and the last time in helmCatalogTypeDescription.

Is there any reason to not keep it here and make the return statements of getCatalogDescription and helmCatalogTypeDescription easier?

jerolimov · 2022-01-24T08:05:40Z

frontend/packages/console-shared/src/components/catalog/CatalogController.tsx

+      );
+    }
+    if (
+      typeof typeExtension?.properties?.catalogDescription !== 'string' &&


Mini nit: If it's a string above, we can skip this check. If typescript needs this I'm also fine with this. This is btw the reason I'm a big fan of else if statements. But our prettier prefers if statements 😄 🤷

Suggested change

typeof typeExtension?.properties?.catalogDescription !== 'string' &&

jerolimov · 2022-01-24T08:06:01Z

/assign

jerolimov

General this PR looks fine. And thanks for this great new tests 👍 🥳

As mentioned in slack and in the code areas below, I have currently 3 concerns:

Should we open the quick start instead of linking the catalog. Def. a PM question, but makes sense for me.
Should we check if the quick start doesn't exist. What should we show then? Can we do all this in helmCatalogTypeDescription? Also a question for PM, but I think we should do this.
Recommend convert helmCatalogTypeDescription to a ~~function~~ compontent.

jerolimov · 2022-01-24T18:04:40Z

frontend/packages/console-shared/src/components/catalog/CatalogController.tsx

-  const title = typeExtension?.properties.title ?? defaultTitle;
-  const description = typeExtension?.properties.catalogDescription ?? defaultDescription;
+  const title = typeExtension?.properties?.title ?? defaultTitle;
+


nit nit nit 😏

Suggested change

jerolimov · 2022-01-24T18:24:22Z

frontend/packages/helm-plugin/src/catalog/utils/catalog-utils.tsx

+
+export const helmCatalogTypeDescription = ( // add the new QS name in the link
+  <Trans ns="helm-plugin">
+    Browse for charts that help manage complex installations and upgrades. Cluster administrators
+    can customize the content made available in the catalog. Alternatively, developers can try{' '}
+    <Link to={'/quickstart?quickstart='}>this quick start</Link> to configure their own custom Helm
+    Chart repository.
+  </Trans>
+);


I have some suggestions and a question for this function:

a) I would suggest converting this to a component. Regardless that the component doesn't have a prop, it "feels" more like a small component.

I tested this also with pseudolocalization and it looks like it was not translated. I don't know if this static field would work if the user switches the language. I think all these cases should just work when converting to a component.

b) After that would suggest moving this out of the utils package into the components package. As said, it feels more like a component than a utils function as you need this render context in your test.

Maybe into its own super small file. Would also suggest exporting it then via helmCatalog in packages/helm-plugin/src/catalog/index.ts. Wdyt?

c) Also mentioned in slack: What happens when the quick start was not found or is disabled? Should we check for the quick start first? 🤯 Is it over-engineered if we do this here?

moved it out of catalog-utils and created a functional component under components folder

jerolimov · 2022-01-24T18:25:09Z

frontend/packages/helm-plugin/src/catalog/utils/catalog-utils.tsx

+  <Trans ns="helm-plugin">
+    Browse for charts that help manage complex installations and upgrades. Cluster administrators
+    can customize the content made available in the catalog. Alternatively, developers can try{' '}
+    <Link to={'/quickstart?quickstart='}>this quick start</Link> to configure their own custom Helm


Asked this in slack already. Do we really want to open the quick start catalog or should we open the quick start?

@jerolimov the quick start name will be added here , which is why I have put the PR on hold

Yes, I added a test quickstart ID. It opens the quick start but is also redirects the user from the "Helm Charts" catalog to the "Quick Starts" catalog. Serena needs to decide if that's okay or not. I would expect that the user stays on the current page and that this link (aka button) just opens the quick start in the sidebar.

jerolimov · 2022-01-25T12:03:08Z

frontend/packages/helm-plugin/src/catalog/components/HelmCatalogTypeDescription.tsx

+
+  const handleOnClick = (event: React.MouseEvent<HTMLElement>) => {
+    if (isModifiedEvent(event)) {
+      event.preventDefault();


When detecting a ctrl+click we should not prevent the link from working. We want that the default browser behavior but we should not handle the click ourself, so that the quickstart is not opened in the current tab.

Suggested change

event.preventDefault();

return;

jerolimov

Thanks for the latest update. Works now fine also with the edge case if the user navigates to a sub item, search for something or open the quick start with ctrl+click. :+1;

/approve
/lgtm

serenamarie125

LGTM

debsmita1 · 2022-01-25T21:37:51Z

updated the helm-plugin.json file

jerolimov · 2022-01-25T23:25:32Z

Serena says lgtm, lets
/unhold
/lgtm

jerolimov · 2022-01-25T23:29:35Z

Propagate from epic:
/label docs-approved
/label px-approved

jerolimov · 2022-01-26T00:42:41Z

Tested on a cluster and works fine when adding the Quick start from openshift/console-operator#631 manually.

jerolimov · 2022-01-26T07:39:37Z

Wait for openshift/console-operator#631
/hold
/label qe-approved

debsmita1 · 2022-01-27T15:38:22Z

we can now safely merge this before openshift/console-operator#631
/unhold

jerolimov

The space before Alternatively and the order of the both strings could make the translation more difficult. But I'm fine here for the moment to get this in and improve this if needed.

Thanks a lot for all the extra rounds, also the small details works great now. "Details matters" 👍

/lgtm

openshift-ci · 2022-01-27T17:03:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: debsmita1, jerolimov, serenamarie125

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

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

openshift-bot · 2022-01-28T03:06:57Z

/retest-required

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

openshift-ci · 2022-01-28T05:29:54Z

@debsmita1: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

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

openshift-ci bot requested review from jcaianirh and spadgett January 21, 2022 16:54

openshift-ci bot added component/helm Related to helm-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Jan 21, 2022

openshift-ci bot assigned rohitkrai03 Jan 21, 2022

openshift-ci bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 21, 2022

openshift-ci bot requested a review from serenamarie125 January 21, 2022 16:55

openshift-ci bot assigned invincibleJai Jan 21, 2022

jerolimov reviewed Jan 24, 2022

View reviewed changes

openshift-ci bot assigned jerolimov Jan 24, 2022

debsmita1 force-pushed the phcr-qs-cta branch from c1f751f to fd6b516 Compare January 24, 2022 15:49

debsmita1 requested a review from jerolimov January 24, 2022 15:52

jerolimov suggested changes Jan 24, 2022

View reviewed changes

debsmita1 force-pushed the phcr-qs-cta branch from fd6b516 to f1c65e9 Compare January 25, 2022 10:09

debsmita1 requested a review from jerolimov January 25, 2022 10:12

debsmita1 force-pushed the phcr-qs-cta branch from f1c65e9 to 4430754 Compare January 25, 2022 11:34

jerolimov reviewed Jan 25, 2022

View reviewed changes

debsmita1 force-pushed the phcr-qs-cta branch from 4430754 to 5ee4ca2 Compare January 25, 2022 13:12

jerolimov approved these changes Jan 25, 2022

View reviewed changes

openshift-ci bot 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 Jan 25, 2022

serenamarie125 approved these changes Jan 25, 2022

View reviewed changes

debsmita1 force-pushed the phcr-qs-cta branch from 5ee4ca2 to f94bfc2 Compare January 25, 2022 21:36

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2022

openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jan 25, 2022

openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Jan 25, 2022

openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. qe-approved Signifies that QE has signed off on this PR labels Jan 26, 2022

add Quick Starts cta to create ProjectHelmChartRepository

7082baa

debsmita1 force-pushed the phcr-qs-cta branch from f94bfc2 to 87d08f4 Compare January 27, 2022 15:35

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 27, 2022

debsmita1 force-pushed the phcr-qs-cta branch from 87d08f4 to 263a8fa Compare January 27, 2022 15:41

check if the namespaced-hcr quickstart is available

406e2ad

debsmita1 force-pushed the phcr-qs-cta branch from 263a8fa to 406e2ad Compare January 27, 2022 16:37

jerolimov approved these changes Jan 27, 2022

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

openshift-merge-robot merged commit 60e2307 into openshift:master Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QuickStarts cta to create ProjectHelmChartRepository in the helm catalog description text #10904

Add QuickStarts cta to create ProjectHelmChartRepository in the helm catalog description text #10904

debsmita1 commented Jan 21, 2022 •

edited

Loading

debsmita1 commented Jan 21, 2022

debsmita1 commented Jan 21, 2022

jerolimov Jan 24, 2022

debsmita1 Jan 24, 2022

jerolimov Jan 24, 2022

debsmita1 Jan 24, 2022

jerolimov commented Jan 24, 2022

jerolimov left a comment •

edited

Loading

jerolimov Jan 24, 2022

jerolimov Jan 24, 2022

debsmita1 Jan 25, 2022

jerolimov Jan 24, 2022

debsmita1 Jan 24, 2022

jerolimov Jan 24, 2022 •

edited

Loading

jerolimov Jan 25, 2022 •

edited

Loading

jerolimov left a comment

serenamarie125 left a comment

debsmita1 commented Jan 25, 2022

jerolimov commented Jan 25, 2022

jerolimov commented Jan 25, 2022

jerolimov commented Jan 26, 2022

jerolimov commented Jan 26, 2022

debsmita1 commented Jan 27, 2022

jerolimov left a comment

openshift-ci bot commented Jan 27, 2022

openshift-bot commented Jan 28, 2022

openshift-ci bot commented Jan 28, 2022

Add QuickStarts cta to create ProjectHelmChartRepository in the helm catalog description text #10904

Add QuickStarts cta to create ProjectHelmChartRepository in the helm catalog description text #10904

Conversation

debsmita1 commented Jan 21, 2022 • edited Loading

debsmita1 commented Jan 21, 2022

debsmita1 commented Jan 21, 2022

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

jerolimov commented Jan 24, 2022

jerolimov left a comment • 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

jerolimov Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

jerolimov Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

jerolimov left a comment

Choose a reason for hiding this comment

serenamarie125 left a comment

Choose a reason for hiding this comment

debsmita1 commented Jan 25, 2022

jerolimov commented Jan 25, 2022

jerolimov commented Jan 25, 2022

jerolimov commented Jan 26, 2022

jerolimov commented Jan 26, 2022

debsmita1 commented Jan 27, 2022

jerolimov left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 27, 2022

openshift-bot commented Jan 28, 2022

openshift-ci bot commented Jan 28, 2022

debsmita1 commented Jan 21, 2022 •

edited

Loading

jerolimov left a comment •

edited

Loading

jerolimov Jan 24, 2022 •

edited

Loading

jerolimov Jan 25, 2022 •

edited

Loading