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.

Sign up for GitHub

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

Jump to bottom

update Installed Operators empty state #1671

Merged
merged 1 commit into from Jul 26, 2019
Merged

update Installed Operators empty state #1671

merged 1 commit into from Jul 26, 2019

Conversation

nicolethoen
Copy link
Member

addresses CONSOLE-1466

Screen Shot 2019-06-04 at 4 15 19 PM

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 4, 2019
alecmerdler
alecmerdler approved these changes Jun 4, 2019
View reviewed changes
Copy link
Contributor

@alecmerdler alecmerdler 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-robot openshift-ci-robot 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 Jun 4, 2019
@openshift-bot
Copy link
Contributor

/retest

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

@spadgett
Copy link
Member

spadgett commented Jun 4, 2019 •
edited

Can you confirm that this doesn't hide the filter bar if you deselect all row filters with items? Otherwise you couldn't clear the filter.

/hold
just to make sure

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2019
@nicolethoen
Copy link
Member Author

Can you confirm that this doesn't hide the filter bar if you deselect all row filters with items? Otherwise you couldn't clear the filter.

@spadgett I put an example of this working below.

Screen Shot 2019-06-05 at 8 47 13 AM

@spadgett spadgett added this to the v4.2 milestone Jun 5, 2019
dtaylor113
dtaylor113 approved these changes Jun 5, 2019
View reviewed changes
Copy link
Contributor

@dtaylor113 dtaylor113 left a comment

Choose a reason for hiding this comment

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

LGTM

@spadgett
Copy link
Member

spadgett commented Jun 6, 2019

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2019
@nicolethoen nicolethoen force-pushed the update_installed_op_empty_state branch from 7b313dc to 395e8ac Compare June 6, 2019 19:29
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 6, 2019
@jhadvig
Copy link
Member

jhadvig commented Jun 7, 2019

/retest

@nicolethoen nicolethoen force-pushed the update_installed_op_empty_state branch from 395e8ac to 10b2528 Compare June 7, 2019 12:44
@nicolethoen
Copy link
Member Author

There is now a difference between the text displayed with no operators installed, and all the installed operators being filtered out.

Screen Shot 2019-06-07 at 8 43 42 AM
Screen Shot 2019-06-07 at 8 43 53 AM

@spadgett
Copy link
Member

spadgett commented Jun 7, 2019

Since we can display entirely different messages, I'd say something like "No Operators match filter" or similar. (Someone from the design side can probably recommend a better message.)

@nicolethoen
Copy link
Member Author

Since we can display entirely different messages, I'd say something like "No Operators match filter" or similar. (Someone from the design side can probably recommend a better message.)

@tlwu2013 Do you have any suggestions?

@nicolethoen nicolethoen force-pushed the update_installed_op_empty_state branch from 1a7d143 to 69db6a9 Compare July 19, 2019 12:16
spadgett
spadgett approved these changes Jul 19, 2019
View reviewed changes
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.

Code looks good, and the removed tests no longer directly apply because the filter bar is no longer visible in these scenarios. Let's open a follow on issue to improve test coverage of custom resources.

/lgtm

frontend/public/components/operator-lifecycle-manager/_operator-lifecycle-manager.scss
@@ -42,6 +42,11 @@
font-size: 13px;
}

.co-clusterserviceversion-empty__state__namespace {
font-weight: bold;
font-style: italic;
Copy link
Member

Choose a reason for hiding this comment

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

nit: alphabetize

@openshift-ci-robot openshift-ci-robot 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 Jul 19, 2019
@openshift-bot
Copy link
Contributor

/retest

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

@spadgett spadgett added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2019
@spadgett
Copy link
Member

It looks like the OLM integration test errors are legimitate, and PR needs rebase.

/hold
until these issues are fixed

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 21, 2019
@nicolethoen nicolethoen force-pushed the update_installed_op_empty_state branch from 69db6a9 to 34a309d Compare July 22, 2019 10:31
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 22, 2019
@nicolethoen nicolethoen force-pushed the update_installed_op_empty_state branch from 34a309d to 7d9e3fb Compare July 22, 2019 11:09
@spadgett
Copy link
Member

@nicolethoen fyi, conflict in list-page.jsx

@nicolethoen nicolethoen force-pushed the update_installed_op_empty_state branch from 7d9e3fb to 1023c73 Compare July 23, 2019 13:44
@nicolethoen
Copy link
Member Author

/retest

@spadgett
Copy link
Member

/hold cancel
/lgtm

@openshift-ci-robot openshift-ci-robot 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 Jul 25, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtaylor113, nicolethoen, 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

@nicolethoen
Copy link
Member Author

/retest

@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit d178255 into openshift:master Jul 26, 2019
nicolethoen
nicolethoen commented Aug 26, 2019
View reviewed changes
frontend/integration-tests/tests/olm/global-installmode.scenario.ts
@@ -170,15 +169,6 @@ describe('Interacting with an `AllNamespaces` install mode Operator (Redis)', ()
expect(crudView.successMessage.getText()).toContain(`${redisEnterpriseCluster} has been updated to version`);
});

it('displays Kubernetes objects associated with the `RedisEnterpriseCluster` in its "Resources" section', async() => {
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 test is the one that needs to be readded once the RedisCluster is adjusted - currently there are no kubernetes objects being created to display in the resources section so this test was not failing because of my change - my change actually highlighted the issue. It had not been effectively testing this scenario before hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecmerdler alecmerdler alecmerdler requested changes

@spadgett spadgett spadgett approved these changes

@dtaylor113 dtaylor113 dtaylor113 approved these changes

@rebeccaalpert rebeccaalpert Awaiting requested review from rebeccaalpert

Assignees

@spadgett spadgett

@alecmerdler alecmerdler

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v4.2
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@nicolethoen @openshift-bot @spadgett @jhadvig @openshift-ci-robot @alecmerdler @dtaylor113 @openshift-merge-robot