feat(dev-catalog): Rename & Reorder existing filters in Dev Catalog #3890

abhinandan13jan · 2020-01-08T10:23:01Z

Addresses - https://issues.redhat.com/browse/ODC-2454

Renames & Reorders existing filters in Dev Catalog
P.S: Helm is not added as it will be added in a later PR #3850

andrewballantyne · 2020-01-08T17:55:30Z

/kind feature

rohitkrai03 · 2020-01-09T13:49:45Z

@abhinandan13jan Your PR seems to have broken the logic for the badges in each card. It was working previously and is there in the design docs of your story as well. But you screencast doesn't have that.

rohitkrai03 · 2020-01-09T13:50:18Z

frontend/public/components/catalog/catalog-items.jsx

Why are you changing kind -> type?

Do you have a use case that fits this type of change? The current acceptance criteria for the story doesn't seem like it needs this change at the moment.

rohitkrai03 · 2020-01-09T13:51:20Z

frontend/public/components/catalog/catalog-items.jsx

I do not get this change? Why was this necessary? The filters worked out previously as intended even without these changes.

The changes here also broke the badge feature. Now there are no badges being shown on the card.

filterGroup has now been changed to type instead of kind. The ability of a type is to hold more than one kind wherever required.

The badges work perfectly with the latest changes

I don't see any badges in the screencast that you've shared with the PR.

Plus, i don't see the point of grouping multiple kinds into a type for filters. In the end it just needs to filter the items in catalog with ability to have multiple filters selected at once. That was already working perfectly fine whichout these changes. Am not sure if am missing something else here. That's why i added comments to the story as well.

that is true. But this adheres to the initial discussion regarding the implementation. The changes can be reverted, if UX confirms that there is no need for grouping of the kinds in the near future. Anyway I'll make the changes go away in that case.

I just want to understand from an implementation point of view what benefits we are getting by grouping kinds here. UX may be thinking it in a way that all unmanaged service are in one group and I agree that's logical but purely from implementation and code changes what will be the benefits?

From a flat data structure you're making it into a nested object struture. And then making the changes to the filtering logic to accomodate the change in data structure. Plus there is no actual grouping of kinds happening here. That doesn't seem right to me.

The existing data structure used for the filters is neither a flat, nor a linear one. It comprises a single layer nesting of objects by filterGroups. For adding flexibility of having multiple kinds per key in a filterGroup, I simply added another layer of objects by nesting the same existing structure of data, which I referred to as grouping.

However, this has been reverted now to one kind per filter.

rohitkrai03 · 2020-01-09T13:52:27Z

frontend/public/components/catalog/catalog-items.jsx

Do not add the helm filter here as there is no helm integration till now. My PR adds the filter for that - #3850

I could remove this. But the filters in the referenced PR wouldn't work with type integration.

This works perfectly with the existing item structure. Hence, if the items with kind HelmChart are added, it will fit in without the need to make any changes in the filters.

I don't see a point adding a filter that doesn't have any item in it.

Just part of the UX for accommodating future needs, IMO.

Well, I think those future needs should be handled by the future story that adds that feature in future. That is actually part of the acceptance criteria for the Helm stories and not part of this one. Even thought the mockups show that in this one as well. We can clarify this with UX.
cc: @serenamarie125

I've put out Helm from the filters

rohitkrai03 · 2020-01-09T13:52:48Z

frontend/public/components/catalog/catalog-items.jsx

This is wrong because Helm charts are not based on ImageStreams

forgot to update the name

Removed all kind groupings

abhinandan13jan · 2020-01-10T11:24:47Z

@rohitkrai03 Please re-review, I have removed all grouping logics. The PR currently Renames & Reorders existing filters in Dev Catalog

christianvogt · 2020-01-10T15:42:32Z

The patternfly catalog, along with operator hub use title case for the Type. I understand that the mock ups show TYPE with a different style. But to stay consistent with Operator Hub I suggest we keep it as Type for now.

andrewballantyne

You're also failing tests in frontend/__tests__/components/catalog.spec.tsx as they checked specific order of the type filters and what the count would be.

serenamarie125

From a UX perspective, the TYPE filters and card badges look as designed. Nice work!

andrewballantyne · 2020-01-10T17:21:45Z

From a UX perspective, the TYPE filters and card badges look as designed. Nice work!

The patternfly catalog, along with operator hub use title case for the Type. I understand that the mock ups show TYPE with a different style. But to stay consistent with Operator Hub I suggest we keep it as Type for now.

@christianvogt and @serenamarie125 I think you're in conflict with your statements.

serenamarie125 · 2020-01-10T20:16:28Z

@christianvogt I didn't see that comment - the style of TYPE should definitely match what it is on OperatorHub, although I'm not sure you changed that

andrewballantyne · 2020-01-10T20:21:39Z

frontend/public/components/catalog/catalog-items.jsx

@serenamarie125 this change makes the two pages unbalanced is what @christianvogt is saying.

Today (before this change) this is the two views:

So changing it to all capitals makes it not match the Operator Hub (and the colour / spacing is different than the designs)

abhinandan13jan · 2020-01-11T11:37:01Z

@christianvogt changed TYPE -> Type,
@andrewballantyne Fixed tests

christianvogt · 2020-01-11T15:19:39Z

/lgtm
/approve

openshift-ci-robot · 2020-01-11T15:19:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinandan13jan, christianvogt, serenamarie125

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

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

openshift-bot · 2020-01-11T16:07:36Z

/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/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 8, 2020

openshift-ci-robot requested review from benjaminapetersen and zherman0 January 8, 2020 10:23

openshift-ci-robot added the component/core Related to console core functionality label Jan 8, 2020

abhinandan13jan changed the title ~~[WIP]feat(dev-catalog): Adjust filters as per developer needs~~ feat(dev-catalog): Adjust filters as per developer needs Jan 8, 2020

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 Jan 8, 2020

abhinandan13jan changed the title ~~feat(dev-catalog): Adjust filters as per developer needs~~ [WIP] feat(dev-catalog): Adjust filters as per developer needs Jan 8, 2020

abhinandan13jan changed the title ~~[WIP] feat(dev-catalog): Adjust filters as per developer needs~~ feat(dev-catalog): Adjust filters as per developer needs Jan 8, 2020

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 Jan 8, 2020

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

abhinandan13jan changed the title ~~feat(dev-catalog): Adjust filters as per developer needs~~ [WIP] feat(dev-catalog): Adjust filters as per developer needs Jan 8, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2020

abhinandan13jan force-pushed the dev-catalog-filter branch from 348b988 to 02e0bcf Compare January 9, 2020 11:12

abhinandan13jan changed the title ~~[WIP] feat(dev-catalog): Adjust filters as per developer needs~~ feat(dev-catalog): Adjust filters as per developer needs Jan 9, 2020

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 Jan 9, 2020

rohitkrai03 suggested changes Jan 9, 2020

View reviewed changes

abhinandan13jan force-pushed the dev-catalog-filter branch 3 times, most recently from 27a4c84 to 7bea248 Compare January 10, 2020 11:22

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2020

abhinandan13jan changed the title ~~feat(dev-catalog): Adjust filters as per developer needs~~ feat(dev-catalog): Rename & Reorder existing filters in Dev Catalog Jan 10, 2020

andrewballantyne suggested changes Jan 10, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 10, 2020

serenamarie125 approved these changes Jan 10, 2020

View reviewed changes

andrewballantyne reviewed Jan 10, 2020

View reviewed changes

abhinandan13jan force-pushed the dev-catalog-filter branch from 7bea248 to 12ccba3 Compare January 11, 2020 09:58

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 11, 2020

feat(dev-catalog): Adjust filters as per developer needs

58eab1b

abhinandan13jan force-pushed the dev-catalog-filter branch from 12ccba3 to 58eab1b Compare January 11, 2020 11:38

openshift-ci-robot assigned christianvogt Jan 11, 2020

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

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

openshift-merge-robot merged commit 9f09cc6 into openshift:master Jan 11, 2020

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

feat(dev-catalog): Rename & Reorder existing filters in Dev Catalog #3890

feat(dev-catalog): Rename & Reorder existing filters in Dev Catalog #3890

Uh oh!

Conversation

abhinandan13jan commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewballantyne commented Jan 8, 2020

Uh oh!

rohitkrai03 commented Jan 9, 2020

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!

rohitkrai03 Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinandan13jan Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

abhinandan13jan commented Jan 10, 2020

Uh oh!

christianvogt commented Jan 10, 2020

Uh oh!

andrewballantyne left a comment

Choose a reason for hiding this comment

Uh oh!

serenamarie125 left a comment

Choose a reason for hiding this comment

Uh oh!

andrewballantyne commented Jan 10, 2020

Uh oh!

serenamarie125 commented Jan 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinandan13jan commented Jan 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christianvogt commented Jan 11, 2020

Uh oh!

abhinandan13jan commented Jan 8, 2020 •

edited

Loading

rohitkrai03 Jan 10, 2020 •

edited

Loading

abhinandan13jan Jan 10, 2020 •

edited

Loading

abhinandan13jan commented Jan 11, 2020 •

edited

Loading