Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1982153: Skip empty categories (id and label) in OperatorHub tab view #9479

Merged
merged 1 commit into from Jul 13, 2021

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Jul 13, 2021

Fixes:
https://issues.redhat.com/browse/ODC-6146
https://bugzilla.redhat.com/show_bug.cgi?id=1982153

Analysis / Root cause:
When running the cypress test developer-catalog-details.feature the test tries to install the OpenShift Serverless operator and fails because the OperatorHub page contains an accessibility issue.

The vertical tab navigation contains a VerticalTabsTab entry (li > a) (rendered in tile-view-page.jsx) which results in an anchor without accessibility infos:

<li class="vertical-tabs-pf-tab text-capitalize" data-test=""><a class="" href="#"></a></li>

(This may depend on the fetched OperatorHub data but happen on fresh clusters started with the cluster-bot.)

Solution Description:
The categories are calculated based on the OperatorHub items, some of them contains an empty category string ({ ... categories: [""] ... }). The new code just skips this empty category.

Screen shots / Gifs for design review:
@openshift/team-devconsole-ux

Skipping the empty string category results in a slightly different (IMHO fixed) UI:

Before there was a space between "All items" and the categories. There is no "Other" category because the entries are shown in this "empty string category.

operatorhub-4 6

On 4.6 it was also possible to select this category. Since 4.7 this is not possible anymore. But the "Other" entry is still missing.

operatorhub-4 6-selected

With this PR the spacing (it was the empty string category) is not shown anymore. The page shows an "Other" entry instead at the end:

operatorhub-shows-other-tab-again

Unit test coverage report:
Added one new unit test for the util function which creates the category object:

 PASS  packages/operator-lifecycle-manager/src/components/operator-hub/operator-hub-items.spec.tsx (6.358s)
  determineCategories
    ✓ should merge categories by name (3ms)
    ✓ should sort categories by name (1ms)
    ✓ should not return categories if there is no defined
    ✓ should not return category if string is empty

Test setup:

  • For an visual test just open the OperatorHub
  • You can also verify the a11y issue with Firefox extension axe - Web Accessibility Testing
  • To validate the cypress test run yarn test-cypress-devconsole and select developer-catalog-details.feature

A11y check
Before:
operatorhub-issue-firefox-axe

After:
operatorhub-fixed-firefox-axe

Cypress notes
This issue was noticed while debugging #9361. This fix focused on this cypress issue:

cypress-test-error-2

After fixing this the tests still has some issues which are already addressed in #9361.

left-over-issue

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci openshift-ci bot added component/olm Related to OLM approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 13, 2021
_.each(items, (item) => {
_.each(item.categories, (category) => {
if (!newCategories[category]) {
if (!newCategories[category] && category) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only line we really need for this fix 🤣

@spadgett
Copy link
Member

@jerolimov Do you mind opening a Bugzilla bug so we can backport this fix?

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 13, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerolimov, spadgett

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 0b4ecab into openshift:master Jul 13, 2021
@jerolimov jerolimov changed the title Skip empty categories (id and label) in OperatorHub tab view Bug 1982153: Skip empty categories (id and label) in OperatorHub tab view Jul 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2021

@jerolimov: All pull requests linked via external trackers have merged:

Bugzilla bug 1982153 has been moved to the MODIFIED state.

In response to this:

Bug 1982153: Skip empty categories (id and label) in OperatorHub tab view

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.

@jerolimov
Copy link
Member Author

/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2021

@jerolimov: Bugzilla bug 1982153 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

/bugzilla refresh

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.

@jerolimov
Copy link
Member Author

@jerolimov Do you mind opening a Bugzilla bug so we can backport this fix?

@spadgett I created a BZ ticket in management console (https://bugzilla.redhat.com/show_bug.cgi?id=1982153), and marked it as low-prio and low-severity because it doesn't break anything. Feel free to change.

It happen since 4.6, but with this low prio, back to which version do you want a backport? I will add a cherrypick then whenever one version is merged.

@spadgett
Copy link
Member

I think just backporting to 4.8 is reasonable.

/cherry-pick release-4.8

@openshift-cherrypick-robot

@spadgett: new pull request created: #9509

In response to this:

I think just backporting to 4.8 is reasonable.

/cherry-pick release-4.8

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.

@spadgett spadgett added this to the v4.9 milestone Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/olm Related to OLM lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants