Dev catalog- group by Operator #3758

debsmita1 · 2019-12-11T19:39:25Z

This PR includes:

adds a Group By dropdown, in the Developer Catalog, with None and Operator as options
when Group By Operator is selected then all the Installed Operators which are owned by a CSV are grouped together, and all the Non-Operators are grouped together separately

Refer JIRA story: https://jira.coreos.com/browse/ODC-2452

Watch video here: https://youtu.be/lbXgLkb05uo

ToDo:

waiting on UX for the final suggestions
handle the scenario where Group By Operator and the categories on top-left of the Dev Catalog are selected

debsmita1 · 2019-12-11T19:40:18Z

^ @openshift/team-devconsole-ux
@christianvogt

serenamarie125 · 2019-12-13T20:28:40Z

Summary of the points we discussed in our meeting:

The order of the items in the Group by Operator should be changed. Operator should be first.
For a "catch all" grouping, for now use Non Operators. We can modify this at a later date if a better term is found
A single grey background should encompass all of the groupings, including the group by titles and tiles
Decrease the amount of space after the group by titles by 50%, this will help to create a stronger visual connection between the group title and its tiles
Move the Group by drop down closer to the filter component. Try using 32px (xl spacer) between the two components.
Once you make these changes, please share again. Currently the group by titles stand out a bit too much. Having them placed on the grey background may help. If not, I'd suggest making them regular font rather than bold.

debsmita1 · 2019-12-17T19:00:16Z

Summary of the points we discussed in our meeting:
* The order of the items in the Group by Operator should be changed. Operator should be first.

Done

* For a "catch all" grouping, for now use Non Operators. We can modify this at a later date if a better term is found

Done

* A single grey background should encompass all of the groupings, including the group by titles and tiles

* Decrease the amount of space after the group by titles by 50%, this will help to create a stronger visual connection between the group title and its tiles

Added 5px space between the title and the group

* Move the Group by drop down closer to the filter component. Try using 32px (xl spacer) between the two components.

Done. Using --pf-global--spacer--xl

* Once you make these changes, please share again. Currently the group by titles stand out a bit too much. Having them placed on the grey background may help. If not, I'd suggest making them regular font rather than bold.

Made the font regular. Using --pf-global--FontSize--lg

@serenamarie125 I have added a youtube link in the PR description. Please let me know if it looks good to you
cc @christianvogt

debsmita1 · 2019-12-18T15:35:27Z

/cc @christianvogt
/cc @abhinandan13jan

rohitkrai03 · 2019-12-18T19:17:07Z

frontend/public/components/utils/tile-view-page.jsx


-const CATEGORY_URL_PARAM = 'category';
-const KEYWORD_URL_PARAM = 'keyword';
+export const FilterType = {


Use enum here.

Better name it FilterTypes.

used const instead of enum because it's giving an error saying 'enum declarations' can only be used in a .ts file cc @christianvogt

oh haha... jsx... didn't pick up on that one :)
Time for an upgrade!

rohitkrai03 · 2019-12-18T19:17:25Z

frontend/public/components/utils/tile-view-page.jsx

+  groupBy: 'groupBy',
+};
+
+export const groupByTypes = {


Same as above.

A better name for the enum would be GroupTypes.

Why not an enum?

rohitkrai03 · 2019-12-18T19:19:45Z

frontend/public/components/utils/tile-view-page.jsx

+export const FilterType = {
+  category: 'category',
+  keyword: 'keyword',
+  groupBy: 'groupBy',


No need to use groupBy everywhere.

Suggested change

groupBy: 'groupBy',

group: 'group',

Grouping should not be a filter.

rohitkrai03 · 2019-12-18T19:21:27Z

frontend/public/components/utils/tile-view-page.jsx

@@ -227,6 +235,17 @@ const filterByGroup = (items, filters) => {
  );
 };

+export const filterByGroupByDropdown = (items, groupBy) => {


Suggested change

export const filterByGroupByDropdown = (items, groupBy) => {

export const filterByGroup = (items, group) => {

It's not really a filter though. It's a sort or grouping. I'd remove filter from this.

Changed it to groupItems

rohitkrai03 · 2019-12-18T19:28:46Z

frontend/public/components/utils/tile-view-page.jsx

+  if (groupBy === groupByTypes.Operator) {
+    const installedOperators = _.filter(items, (item) => item.kind === 'InstalledOperator');
+    const nonOperators = _.filter(items, (item) => item.kind !== 'InstalledOperator');
+    const groupedOperators = _.groupBy(installedOperators, (item) => item.obj.csv.metadata.name);


Maybe we can sanitize the operator name somehow to make it look better on the UI. The title currently looks really odd.

@rohitkrai03 Do you think the version is making it look odd? cc @serenamarie125

@rohitkrai03 Let me know if this looks okay

Yeah, this looks much better.

rohitkrai03 · 2019-12-18T19:29:30Z

frontend/public/components/utils/tile-view-page.jsx

+    const installedOperators = _.filter(items, (item) => item.kind === 'InstalledOperator');
+    const nonOperators = _.filter(items, (item) => item.kind !== 'InstalledOperator');
+    const groupedOperators = _.groupBy(installedOperators, (item) => item.obj.csv.metadata.name);
+    const groupAllItems = { ...groupedOperators, 'Non-Operators': nonOperators };


Why add a hyphen in between when its a string?

Suggested change

const groupAllItems = { ...groupedOperators, 'Non-Operators': nonOperators };

const groupAllItems = { ...groupedOperators, 'Non Operators': nonOperators };

rohitkrai03 · 2019-12-18T19:31:25Z

frontend/public/components/utils/tile-view-page.jsx

@@ -310,10 +329,13 @@ const determineAvailableFilters = (initialFilters, items, filterGroups) => {
  return filters;
 };

-const getActiveFilters = (keywordFilter, groupFilters, activeFilters) => {
+const getActiveFilters = (keywordFilter, groupByFilter, groupFilters, activeFilters) => {


Oh, i see why you had to name it groupBy everywhere.

christianvogt · 2019-12-18T21:00:57Z

frontend/public/components/utils/tile-view-page.jsx

+  groupBy: 'groupBy',
+};
+
+export const groupByTypes = {


Why not an enum?

christianvogt · 2019-12-18T21:01:29Z

frontend/public/components/utils/tile-view-page.jsx

+      (value, key) =>
+        value.length > 0 && (
+          <>
+            <div className="co-catalog-page__group-title">


Use a standard header? h2 perhaps

christianvogt · 2019-12-18T21:02:33Z

frontend/public/components/utils/tile-view-page.jsx

+              className="catalog-tile-view-pf catalog-tile-view-pf-no-categories co-catalog-page__grid"
+            >
+              {_.map(value, (item) => (
+                <GalleryItem key={`gallery-${item.obj.metadata.uid}`}>


This isn't consistent with the other gallery item.
It's also duplicate code that should be extracted and reused.

Moved this part to renderGalleryItem and reusing it whereever required

christianvogt · 2019-12-18T21:03:20Z

frontend/public/components/utils/tile-view-page.jsx

+            </div>
+            <Gallery
+              gutter="sm"
+              className="catalog-tile-view-pf catalog-tile-view-pf-no-categories co-catalog-page__grid"


Gallery shouldn't have co-catalog-page__grid here.

christianvogt · 2019-12-18T21:03:58Z

frontend/public/components/utils/tile-view-page.jsx

+            ) : (
+              <Gallery
+                gutter="sm"
+                className="catalog-tile-view-pf catalog-tile-view-pf-no-categories co-catalog-page__grid"


Move co-catalog-page__grid to a parent div so that this case and the grouping case use the same parent with this class.

christianvogt · 2019-12-18T21:11:01Z

frontend/public/components/utils/tile-view-page.jsx

+    if (filterType === FilterType.keyword) {
+      updateURLParams(FilterType.keyword, `${value}`);
+    } else if (filterType === FilterType.groupBy) {
+      updateURLParams(FilterType.groupBy, `${value}`);


Grouping is not a filter and should be handled separately.

christianvogt · 2019-12-18T21:14:02Z

frontend/public/components/utils/tile-view-page.jsx

@@ -227,6 +235,17 @@ const filterByGroup = (items, filters) => {
  );
 };

+export const filterByGroupByDropdown = (items, groupBy) => {


It's not really a filter though. It's a sort or grouping. I'd remove filter from this.

christianvogt · 2019-12-18T21:15:08Z

frontend/public/components/utils/tile-view-page.jsx

+  activeFilters.groupBy.value = groupByFilter || '';
+  activeFilters.groupBy.active = !!groupByFilter;


I'm confused. Group by does not actually filter anything, it only affects the visual appearance of the items.
This should not be mixed in with the filters on the page.

@christianvogt Initially the ask was to display only the Installed Operators in groups. Later on when we decided to add the Non operators grouped together I should have updated the variable names which I missed

moved it out of activeFilters

christianvogt · 2019-12-18T21:16:20Z

frontend/public/components/utils/tile-view-page.jsx

+  } else if (filterType === FilterType.groupBy) {
+    _.set(activeFilters, 'groupBy.value', value);
+    _.set(activeFilters, 'groupBy.active', !!value);


Grouping is not a filter.

christianvogt · 2019-12-18T21:16:50Z

frontend/public/components/utils/tile-view-page.jsx

+export const FilterType = {
+  category: 'category',
+  keyword: 'keyword',
+  groupBy: 'groupBy',


Grouping should not be a filter.

benjaminapetersen · 2019-12-18T21:19:27Z

/assign @christianvogt

I believe.
Also helpful to add a screenshot to your initial description, thx!

rohitkrai03

/lgtm

rohitkrai03 · 2019-12-27T14:11:02Z

/assign @christianvogt

debsmita1 · 2019-12-27T14:18:01Z

/test e2e-gcp-console

debsmita1 · 2019-12-30T15:07:34Z

/test analyze

andrewballantyne

I think we should look to also convert these to Typescript, but I believe there is a dependency on other public files, so perhaps that's out of scope for this work.

andrewballantyne · 2019-12-30T19:44:21Z

frontend/public/components/utils/tile-view-page.jsx

  const selectedCategoryId = categoryParam || 'all';
-
+  const groupBy = searchParams.get('groupBy') || 'None';


You use 'None' quite a few times, please make it a constant to avoid the magic string disconnect.

andrewballantyne · 2019-12-30T20:06:51Z

frontend/public/components/utils/tile-view-page.jsx

+          <>
+            <Title className="co-catalog-page__group-title" headingLevel="h2" size="lg">
+              {key} ({_.size(value)})
+            </Title>
+            {this.renderItems(value, renderTile)}
+            <br />
+          </>


I think this can be better represented with css rather than a br. I think brs are old fashioned and we lose the fidelity to control the gap.

Also, you lack the key prop. Anytime you do map you need to use the key prop on returned JSX.

added key prop

removed
and added a css class to handle the space between the groups

andrewballantyne · 2020-01-02T16:12:03Z

frontend/public/components/utils/tile-view-page.jsx

@@ -467,10 +468,12 @@ const defaultFilters = {
  },
 };

+const none = 'None';


Not quite what I meant :) For one, this will fail the linter as it's defined after its use... For two, this won't help using two different variables to match. You either need to use this exclusively... or use the enum we talked about.

Sorry for that. Now using groupByTypes.None everywhere instead of 'None'

andrewballantyne

/lgtm
/approve

I don't see any issues with the OperatorHub around this change. Looks good in the Catalog too. Will probably need a UX look-over at some point too.

/assign @christianvogt

andrewballantyne · 2020-01-02T17:01:09Z

/hold

e2e tests are still failing, no need for the bot to hammer after it fails.

andrewballantyne · 2020-01-02T19:44:46Z

/kind feature

christianvogt · 2020-01-02T20:04:07Z

/approve

openshift-ci-robot · 2020-01-02T20:04:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, debsmita1, rohitkrai03

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

spadgett · 2020-01-02T23:26:09Z

/hold cancel
/retest
#3847 has fixed the e2e tests

openshift-bot · 2020-01-02T23:27:58Z

/retest

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

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/core Related to console core functionality labels Dec 11, 2019

openshift-ci-robot requested review from benjaminapetersen and jcaianirh December 11, 2019 19:39

debsmita1 force-pushed the group-by-operator branch from 1c9ca32 to 7dc38a4 Compare December 12, 2019 07:00

debsmita1 force-pushed the group-by-operator branch 3 times, most recently from 9f90bd0 to 409eced Compare December 17, 2019 18:42

debsmita1 force-pushed the group-by-operator branch 3 times, most recently from 1c586dc to 236e46e Compare December 18, 2019 11:25

debsmita1 changed the title ~~[WIP]: Dev catalog- group by Operator~~ Dev catalog- group by Operator Dec 18, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 18, 2019

debsmita1 force-pushed the group-by-operator branch 2 times, most recently from 8b72a4c to 060f6c8 Compare December 18, 2019 12:04

openshift-ci-robot requested review from abhinandan13jan and christianvogt December 18, 2019 15:35

rohitkrai03 suggested changes Dec 18, 2019

View reviewed changes

christianvogt reviewed Dec 18, 2019

View reviewed changes

openshift-ci-robot assigned christianvogt Dec 18, 2019

debsmita1 force-pushed the group-by-operator branch 4 times, most recently from 3f4d64c to 25a81b8 Compare December 19, 2019 15:53

debsmita1 force-pushed the group-by-operator branch 2 times, most recently from c0bb1d9 to a47ee18 Compare December 24, 2019 12:53

rohitkrai03 approved these changes Dec 27, 2019

View reviewed changes

openshift-ci-robot assigned rohitkrai03 Dec 27, 2019

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

andrewballantyne suggested changes Dec 30, 2019

View reviewed changes

debsmita1 force-pushed the group-by-operator branch from a47ee18 to 10a7da9 Compare January 2, 2020 16:00

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 2, 2020

debsmita1 force-pushed the group-by-operator branch from 10a7da9 to d560995 Compare January 2, 2020 16:01

andrewballantyne suggested changes Jan 2, 2020

View reviewed changes

dev catalog group by operator

45dcc58

debsmita1 force-pushed the group-by-operator branch from d560995 to 45dcc58 Compare January 2, 2020 16:43

andrewballantyne approved these changes Jan 2, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 2, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 2, 2020

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

openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 2, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2020

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

openshift-merge-robot merged commit 17196ae into openshift:master Jan 3, 2020

spadgett added this to the v4.4 milestone Jan 3, 2020

	export const filterByGroupByDropdown = (items, groupBy) => {
	export const filterByGroup = (items, group) => {

	const groupAllItems = { ...groupedOperators, 'Non-Operators': nonOperators };
	const groupAllItems = { ...groupedOperators, 'Non Operators': nonOperators };

		activeFilters.groupBy.value = groupByFilter \|\| '';
		activeFilters.groupBy.active = !!groupByFilter;

		const selectedCategoryId = categoryParam \|\| 'all';

		const groupBy = searchParams.get('groupBy') \|\| 'None';

Dev catalog- group by Operator #3758

Dev catalog- group by Operator #3758

Conversation

debsmita1 commented Dec 11, 2019 • edited

debsmita1 commented Dec 11, 2019 • edited

serenamarie125 commented Dec 13, 2019

debsmita1 commented Dec 17, 2019

debsmita1 commented Dec 18, 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

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

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

benjaminapetersen commented Dec 18, 2019 • edited

rohitkrai03 left a comment

Choose a reason for hiding this comment

rohitkrai03 commented Dec 27, 2019

debsmita1 commented Dec 27, 2019

debsmita1 commented Dec 30, 2019

andrewballantyne 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

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Jan 2, 2020

andrewballantyne commented Jan 2, 2020

christianvogt commented Jan 2, 2020

openshift-ci-robot commented Jan 2, 2020

spadgett commented Jan 2, 2020

openshift-bot commented Jan 2, 2020

debsmita1 commented Dec 11, 2019 •

edited

debsmita1 commented Dec 11, 2019 •

edited

benjaminapetersen commented Dec 18, 2019 •

edited