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

Advanced search #7739

Merged
merged 14 commits into from
Aug 6, 2020
Merged

Advanced search #7739

merged 14 commits into from
Aug 6, 2020

Conversation

jlmitch5
Copy link
Contributor

@jlmitch5 jlmitch5 commented Jul 28, 2020 •
edited
Loading

This PR adds advanced search support to the lists in UI next.

To access advanced search, it is the last key in the search key dropdown of any list, just click on it:

Screen Shot 2020-07-28 at 2 59 59 PM

When you do so, the search input group will change to 3 typeahead selects and a text input

Screen Shot 2020-07-28 at 3 00 10 PM

All search-component related updates are in the first commit of this PR

Screen Shot 2020-07-28 at 3 01 47 PM

The first typeahead is the optional set type selector. you can choose these different set types to change the set of results returned. for example if you search for or__foo: bar and or__foo: baz, it will return results where the foo key is either foo or baz. While the api doesn't explicitly support typing and__ (it is the default set type when no set type prefix is entered), it didn't really make sense to not include it in the descriptions here, so if the user adds it, it will get stripped out when sent to the api. It would be better if the api allowed it (like they do for the __exact default lookup), but I think it works for now.

Screen Shot 2020-07-28 at 3 06 29 PM

The second typeahead is the key selector. It allows you to select from a set of keys which are returned from the options request for the list, as well type in a key that is not listed. It is required, and when a key is selected the value text input is enabled.

#7741 -- Currently, there are two sets of keys displayed in the options for this typeahead--the direct keys of the resource, and the related search fields. Since there is currently a bug with PF support ( pr open so should be fixed soon -- patternfly/patternfly-react#4622 ) of select groups with typeaheads, I have to concat and dedup these two together. The related search fields are special in that when no prefix or lookup is specified, we want to allow the user to access the fuzzy search for these related resources, so I append the __search lookup when that is the case. Because there are some cases where a related key is both a direct key (usually the id of which resource it is connected to), as well as a related search key with which you can search the fields of that resource, I do not do the __search appending if it is in both sets of options.

Screen Shot 2020-07-28 at 3 16 13 PM

The third typeahead are the lookups, which allows you to set specific ways to check the value against the key.

All list changes across the app are in the second commit of this pr

I had to make a couple of adjustments to the simple search implementation to help things make sense now that advanced search results are possible.

The first is that the UX-enhancement of adding the i18n'd, sentence-cased name of the search keys for simple search'd tags could get a bit confusing when there are also chip groups of these keys but with different modifiers. @trahman73 and I came up with the idea of including this i18n'd label, but also including what specific key is being searched so that it feels a bit more clear. For example:

Screen Shot 2020-07-28 at 3 24 08 PM

The second is a bit of magic that I couldn't figure out how to keep without really complicating the code. Basically the icontains and or modifiers were added to text and select keys implicitly based on the qsConfig. with the addition of the new advanced search keys the way I was making the groups of chips based on the keys because super complicated, and I couldn't figure out how to save this bit of magic without making things super brittle. So I added these modifiers explicitly to the searchColumn keys cross ui_next. I don't love that I had to do this, and if anyone could figure out a nicer way that we could do this, I'm definitely for adding back in this developer experience improvement.

#7740 -- There are a handful of screens that don't have anything populated in the options for the key typeahead, as they don’t use the useRequest hook yet. In terms of work, I think it makes sense that they should be converted first so that they can grab the options request to get possible advanced keys.

  • UserList.jsx
  • WFJTList
  • (Templates) ProjectsList.jsx
  • JTList
  • InventorySourcesList.jsx
  • ProjectJobTemplatesList.jsx
  • OrganizationTeamList.jsx
  • InventoryGroupsList.jsx
  • ResourceAccessList.jsx / AddResourceRole lists
  • UserAndTeamAccessAdd /SelectResourceStep lists*
  • AssociateModal lists*
  • NotificationList.jsx
  • OrganizationLookup

*these actually need to be passed an additional options request from the places they are called, they have useRequest in use right now, so bit of a different conversion

A note on responsiveness

The only thing left is figuring out mid-size screen responsiveness. I've got things looking nice for desktop view:

Screen Shot 2020-07-28 at 3 30 16 PM

and mobile view:

Screen Shot 2020-07-28 at 3 30 39 PM

but the spacing gets bad for middle-width views:

Screen Shot 2020-07-28 at 3 31 30 PM

Did figure out a workaround for this (https://stackoverflow.com/questions/35578404/giving-wrapped-flexbox-items-vertical-spacing/44523788) but I'm not adding it now in hopes it is fixed on the PF side. @marshmalien made the discovery that flexbox will allow for a wrapping row gap (hopefully common place soon!) which would solve the problems:

https://developer.mozilla.org/en-US/docs/Web/CSS/gap#Browser_compatibility

The hack that works is:

.pf-c-toolbar__content-section {
  margin-top: -20px;
}
.pf-c-toolbar__content-section > * {
  margin-top: 20px;
}

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul

This comment has been minimized.

Sign in to view
@jlmitch5
Copy link
Contributor Author

recheck

@softwarefactory-project-zuul

This comment has been minimized.

Sign in to view
@softwarefactory-project-zuul

This comment has been minimized.

Sign in to view
@mabashian mabashian mentioned this pull request Jul 31, 2020
Closed
@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@softwarefactory-project-zuul

This comment has been minimized.

Sign in to view
@unlikelyzero
Copy link

unlikelyzero commented Aug 4, 2020 •
edited
Loading

Issues:

  • File separate: SelectAll checkbox gets smushed out of alignment when browser width less than 1340 pix

Screen Shot 2020-08-04 at 2 11 01 PM

  • Unable to clear all filters after generating a 400 response. Create an advanced filter with 'and' 'id' 'isnul' 'asdf'. See 400 code. Attempt to 'Clear all filters'

Screen Shot 2020-08-04 at 6 08 37 PM

@softwarefactory-project-zuul

This comment has been minimized.

Sign in to view
@softwarefactory-project-zuul

This comment has been minimized.

Sign in to view
@softwarefactory-project-zuul

This comment has been minimized.

Sign in to view
@unlikelyzero
Copy link

unlikelyzero commented Aug 5, 2020 •
edited
Loading

@jlmitch5 should I be able to see this Keytype on JTs?
Screen Shot 2020-08-05 at 12 30 23 PM

When using typeahead, should I see the last option to create the typeahead value?
Screen Shot 2020-08-05 at 12 32 59 PM

Should refreshing a JT Advanced filter page include the original filter or clear all filters?
Screen Shot 2020-08-05 at 12 35 49 PM

Is there a working or/not query I could generate?

Unable to perform advanced search on Credentials, Users, Credential Types, Instance Groups

500 Error when attempting to perform organization.id__iexact=0
Screen Shot 2020-08-05 at 12 47 22 PM

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

marshmalien
marshmalien reviewed Aug 6, 2020
View reviewed changes
awx/ui_next/src/components/Lookup/InventoryScriptLookup.jsx Outdated
@@ -0,0 +1,156 @@
import React, { useCallback, useEffect } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

We recently removed InventoryScripts here: #7577
This file probably snuck in during a rebase, so we should remove it.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

marshmalien
marshmalien approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member

@marshmalien marshmalien left a comment

Choose a reason for hiding this comment

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

Awesome stuff. I really like the direction advanced search is going and it'll be exciting to get this into the UI 🎉

@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

John Mitchell and others added 14 commits August 6, 2020 12:17
Add advanced search to UI
b46a872
Updates to support advanced search changes:
5ffc2e4 
- make set type and lookup prefixes/suffixes on searchColumns explicitly defined
- send possible search keys from options requests on (most) lists
fix duplicate variable and key usng array index issues
2229d43
Add AdvancedSearch propTypes and defaultProps
dc2bf50
Move Search to hooks and excise PF Dropdown in favor of Select
36585ad
fix merge conflicts and failing test
ede1260
fix AddRersourceRole sort column
0876b94
add selectors for cypress tests
5b362ef
add back in searchable keys props to user token list
f15325a
make sortColumnKey error message more clear
495b414
update searchablekeys prop names for project lookup
f24654f
make name default searchColumn for ProjectJobTemplatesList. also add …
f8bd8ab 
…helpful error message to tell you this is the issue
update rest of lookups to use correct searchableKeys props
cbea77a
delete inadverdently added back InventoryScriptLookup file
61ce8cb
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 91df10d into ansible:devel Aug 6, 2020
@mabashian mabashian mentioned this pull request Aug 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marshmalien marshmalien marshmalien approved these changes

@mabashian mabashian Awaiting requested review from mabashian

@chrismeyersfsu chrismeyersfsu Awaiting requested review from chrismeyersfsu

@keithjgrant keithjgrant Awaiting requested review from keithjgrant

@unlikelyzero unlikelyzero Awaiting requested review from unlikelyzero

@dsesami dsesami Awaiting requested review from dsesami

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jlmitch5 @unlikelyzero @marshmalien @jakemcdermott