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

[WIP] Change search to handle multiple resources #3719

Closed
wants to merge 2 commits into from

Conversation

zherman0
Copy link
Member

@zherman0 zherman0 commented Dec 7, 2019

Change search page to allow for multiple resources to be selected at one time and searched using query.
Functionality this is complete, but visually we might want to make more tweaks.
Version 2: (Using PF4 Select with multi and typeahead)
There are still several visual issues and there is a difference between PF 3.112 (consle version) and PF 3.124 (current).
Images with 3.112
multiselect-12
multiselect-11
multiselect-10
Filter Removed:
multselect-no-filter

Images with PF 3.124
multiselect-1
multiselect-2
multiselect-3

Version1: (non PF dropdown)
I tried several ways to unselect resources but decided on a checkbox for UX. I don't love the appearance so I am open to comments. I did try simply highlighting the selected row in the dropdown but was not sure that it would be obvious to users they needed to click on it again to unselect the item. I also added a close button on each resource but the title area of the dropdown is controlled by the generic Dropdown component and the close button was not clickable since it invoked the dropdown. I should have taken more pics along the way but I did not.
Here are the current images:
multisearch-1
multisearch-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 Dec 7, 2019
@zherman0
Copy link
Member Author

zherman0 commented Dec 7, 2019

@spadgett - first version. Please review when you have some time.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zherman0
To complete the pull request process, please assign jhadvig
You can assign the PR to them by writing /assign @jhadvig in a comment when ready.

The full list of commands accepted by this bot can be found 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

@zherman0
Copy link
Member Author

zherman0 commented Dec 8, 2019

/retest

1 similar comment
@zherman0
Copy link
Member Author

zherman0 commented Dec 9, 2019

/retest

@zherman0 zherman0 changed the title [WIP] Change search to handle multiple resources Change search to handle multiple resources Dec 9, 2019
@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 Dec 9, 2019
@spadgett
Copy link
Member

spadgett commented Dec 9, 2019

I wonder if we should just use Patternfly Select with SelectVariant.typeaheadMulti.

https://www.patternfly.org/v4/documentation/react/components/select#multiple

If we use our own, I would just show a count for the number selected like the Patternfly widget.

PatternFly 4 2019-12-09 14-02-09

@spadgett
Copy link
Member

spadgett commented Dec 9, 2019

We might skip preselecting Service when we make this change since it's extra clicks to manually deselect it when you want something else.

@spadgett
Copy link
Member

spadgett commented Dec 9, 2019

We should try to have one text filter for all types instead of repeating it.

@zherman0 zherman0 changed the title Change search to handle multiple resources [WIP] Change search to handle multiple resources Dec 11, 2019
@openshift-ci-robot 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 Dec 11, 2019
@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 Dec 12, 2019
@zherman0
Copy link
Member Author

/retest

1 similar comment
@zherman0
Copy link
Member Author

/retest

@sg00dwin
Copy link
Member

sg00dwin commented Dec 17, 2019

UX and visual issues to be addressed

Screen Shot 2019-12-16 at 15 00 54
Screen Shot 2019-12-16 at 14 54 28


  • Tab shortcut closes the dropdown
  • Arrow shortcut rotates though select options, but :hover :focus state highlighting doesn't work
  • Left side Resource name is cut off

Search-keyboard-navigation1


Search Results

  • Many resource selections can cause actual results to be below the scroll
    Search-keyboard-navigation2

cc @spadgett @benjaminapetersen @rhamilto

@openshift-ci-robot
Copy link
Contributor

@zherman0: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/frontend c07b46b link /test frontend
ci/prow/e2e-gcp-console c07b46b link /test e2e-gcp-console

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@benjaminapetersen
Copy link
Contributor

/hold
As we look at the issues to address. @zherman0 was going to open a second PR with his initial simplified impl as a fallback.

@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 Dec 19, 2019
@zherman0
Copy link
Member Author

zherman0 commented Jan 7, 2020

patternfly/patternfly-react#3457 has been created to tweak how this PF component works which should make this more usable. @spadgett @benjaminapetersen

@zherman0
Copy link
Member Author

Closing in favor of solution #3875

@zherman0 zherman0 closed this Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/core Related to console core functionality do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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

5 participants