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

Change search to handle multiple resources and update search page #3875

Merged
merged 1 commit into from Jan 25, 2020

Conversation

zherman0
Copy link
Member

@zherman0 zherman0 commented Jan 6, 2020

This updates the search page to transition to the new UX desgin. https://openshift.github.io/openshift-origin-design/admin-perspective/future-openshift/search/search
This will be a multi step process spread over version 4.4 and 4.5. This is step 1.
Instead of using patternfly4 select options, it uses an in house dropdown solution that includes a typeahead filter. (This is an alternative solution to PR3719, which is now closed)
Screenshot_2020-01-17 Search · OKD(1)
Screenshot_2020-01-17 Search · OKD(2)
Screenshot_2020-01-17 Search · OKD
Peek 2020-01-17 09-29

Since Events uses the same component, it too as changed. The Events sections are now labelled to identify what is being shown and multiple resources can be selected.
Screenshot_2020-01-20 Events · OKD
Screenshot_2020-01-20 Events · OKD(1)
Screenshot_2020-01-20 Events · OKD(2)

@openshift-ci-robot 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 Jan 6, 2020
@zherman0 zherman0 changed the title [WIP] Alternative 2 to PR3719: Homemade solution [WIP] Alternative 2 for new Search dropdown (alt for PR3719: Homemade solution) Jan 6, 2020
@zherman0
Copy link
Member Author

zherman0 commented Jan 7, 2020

@spadgett @benjaminapetersen - Our home grown non-PF solution for the search dropdown. This is the third PR for search options.

@zherman0 zherman0 changed the title [WIP] Alternative 2 for new Search dropdown (alt for PR3719: Homemade solution) [WIP] Change search to handle multiple resources and update search page Jan 10, 2020
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2020
@zherman0 zherman0 force-pushed the multi-homemade branch 2 times, most recently from e4b1dda to 248c032 Compare January 14, 2020 19:16
@openshift-ci-robot openshift-ci-robot added the component/olm Related to OLM label Jan 14, 2020
@sg00dwin
Copy link
Member

@zherman0 here's my branch with updates I mentioned I worked on yesterday. There's some conflicts with your most recent commit, which I think where addressing the same issue that I did. So I didn't open a pr against yours.

sg00dwin@e9eb73c

gif showing how it looks
multi-search

@zherman0
Copy link
Member Author

There is an open issue with the search page that should be investigated with this work: https://issues.redhat.com/browse/CONSOLE-1981

@zherman0
Copy link
Member Author

/retest

@zherman0 zherman0 force-pushed the multi-homemade branch 3 times, most recently from f0fcaac to 2456d0c Compare January 17, 2020 06:18
@zherman0
Copy link
Member Author

/retest

@zherman0 zherman0 changed the title [WIP] Change search to handle multiple resources and update search page Change search to handle multiple resources and update search page Jan 17, 2020
@openshift-ci-robot 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 17, 2020
@zherman0
Copy link
Member Author

@spadgett @benjaminapetersen - This is ready for review. While this was fairly large, the fact I changed search.jsx to search.tsx makes it a lot to look over. Thanks.

frontend/public/components/events.jsx Outdated Show resolved Hide resolved
frontend/public/components/events.jsx Outdated Show resolved Hide resolved
frontend/public/components/factory/list-page.jsx Outdated Show resolved Hide resolved
frontend/public/components/factory/list-page.jsx Outdated Show resolved Hide resolved
frontend/public/components/factory/list-page.jsx Outdated Show resolved Hide resolved
frontend/public/components/search.tsx Outdated Show resolved Hide resolved
frontend/public/components/search.tsx Outdated Show resolved Hide resolved
frontend/public/components/search.tsx Outdated Show resolved Hide resolved
frontend/public/components/search.tsx Outdated Show resolved Hide resolved
frontend/public/components/search.tsx Outdated Show resolved Hide resolved
@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Jan 24, 2020

Experientally, I would prefer if it didn't automatically close on every checkbox select. Its a bit cumbersome to select multiple items:

2020-01-24 11 52 13

  • Open bugzilla if we want to follow-up on this instead of fix on first pass.

@zherman0
Copy link
Member Author

zherman0 commented Jan 24, 2020

Experientally, I would prefer if it didn't automatically close on every checkbox select. Its a bit cumbersome to select multiple items:

* [ ]  Open bugzilla if we want to follow-up on this instead of fix on first pass.

@benjaminapetersen - I did discuss this with UX. The thought was that most users would probably want only one item since that is how it works today. If we force a user to close after one click then that greatly affects their experience. Now, if 80% of people do multiple search, then we should definitely leave it open.

@zherman0
Copy link
Member Author

/retest

@benjaminapetersen
Copy link
Contributor

@zherman0 I would think "click outside" would close effectively, rather than require an explicit "close".

@benjaminapetersen
Copy link
Contributor

benjaminapetersen commented Jan 24, 2020

It seems to be && not ||, requiring that a resource (or several) are selected before the label selector will work:

Screen Shot 2020-01-24 at 1 16 35 PM

I can imagine users wanting to search by label primarily, however.

  • Decide if this is correct, or if we should fix/make a bug for follow.
  • Make sure the label syntax needed is clear. I assumed I was providing incorrect syntax so added a number of labels until I realized I had to have a resource type selected.

@zherman0
Copy link
Member Author

/retest

@spadgett
Copy link
Member

It seems to be && not ||, requiring that a resource (or several) are selected before the label selector will work:

There's no API to search across all resources by label. I'm not sure it's practical to run a separate list for every resource even with a label selector, so we ask the user to pick a resource first.

@benjaminapetersen
Copy link
Contributor

Yeah thought about that after a bit, there is not kubectl/oc query with a label that doesn't require a get x resource. So we don't have an API either.

@benjaminapetersen
Copy link
Contributor

/lgtm
/hold

I think we are good to go with this one (if CI opens up). Can address any follows as bugs or future Jira if needed.

@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 Jan 25, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, zherman0

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2020
@benjaminapetersen
Copy link
Contributor

Holding on #4065

@spadgett
Copy link
Member

/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 Jan 25, 2020
@openshift-bot
Copy link
Contributor

/retest

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

6 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-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 5b6344d into openshift:master Jan 25, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
@zherman0 zherman0 deleted the multi-homemade branch January 27, 2020 22:47
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/core Related to console core functionality component/olm Related to OLM lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants