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

[24.0] Fix history filters taking up space in GridList #17652

Merged

Conversation

ahmedhamidawan
Copy link
Member

@ahmedhamidawan ahmedhamidawan commented Mar 8, 2024

Enhancement Added here to the Grids and Workflow List:

Fixes #17618

compact_filter_menu.mp4

Additional feature also added (but not enabled anywhere):

Add a popover version of the FilterMenu component. This can be enabled via the isPopover prop.

Before After
prepopover_filter_menu.mp4
withpopover_filter_menu.mp4

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

@ahmedhamidawan
Copy link
Member Author

@martenson

As of the popover this might be one of the better uses for it but I would be concerned about accessibility.

It was impossible to tab over to the popover once it is shown, so instead I used the @shown and @hidden methods from BPopover to focus on the first input field in the menu when opened, and on the advanced filters button when the popover is closed (can be closed with keydown.esc)

@mvdbeek
Copy link
Member

mvdbeek commented Mar 11, 2024

I think the persistent view is more functional, any chance those filters can be combined into the slightly more user-friendly combinations that what we've had previously ?

Screenshot 2024-03-11 at 10 22 14

vs

Screenshot 2024-03-11 at 10 22 25

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Mar 20, 2024

What do we think about this:

compact_filter_menu.mp4

A slightly more compact version of the FilterMenu which applies filters immediately and doesn't automatically collapse when a filter is applied. This though defeats the purpose of having a Search button at the bottom, so I removed it for this version of the FilterMenu.

@mvdbeek
Copy link
Member

mvdbeek commented Mar 20, 2024

Looks great!

@ahmedhamidawan
Copy link
Member Author

ahmedhamidawan commented Mar 20, 2024

@mvdbeek thanks! Note that i updated the screencast above

Unfortunately, there is one functionality that kind of breaks, i.e.: When you have the advanced filters opened, and you try to change the filterText, it automatically removes invalid filters while you type: because it is applying the values of the filters from the text field to the v-models in the filters:

deletes_filters_unexpectedly.mp4

However, one must note that users will typically not edit the text in the main search field when they have filters applied and they would probably edit those individual fields.

@ahmedhamidawan
Copy link
Member Author

To counter this, the main search field can be disabled in case the menu is expanded:

compact_filter_menu_field_disabled.mp4

@martenson
Copy link
Member

I did not want to bring it up, since this came a long way already, but if it is giving you any problems consider dropping the "name" search field all together. I have trouble imagining a use case where "name+tag" search field wouldn't find something that "name" search field does.

@ahmedhamidawan
Copy link
Member Author

I did not want to bring it up, since this came a long way already, but if it is giving you any problems consider dropping the "name" search field all together. I have trouble imagining a use case where "name+tag" search field wouldn't find something that "name" search field does.

By default, the search query (without filters) looks at name and tag (and annotation etc?) fields in the backend; and an explicit name filter obviously only looks at the name field. That's why we have separate name filters for all grids and workflow list

For some classes like HistoryFilters for the history panel, the Filtering class checks this prop:
https://github.com/ahmedhamidawan/galaxy/blob/dev/client/src/utils/filtering.ts#L235-L238

 * @param nameMatching: Whether to apply name filter for unspecified filterText
 *                      (e.g. filterText = 'foo' -> 'name:foo').
 *                      Typically, when this is false, we index every field in
 *                      the backend for unspecified filterText.

and if there is no filter in the query, it automatically assumes there is a name filter because that's how the backend HistoryContents filters work

So, I don't know if this addresses your concern but this is the way the no-filter query works for the grids.

I reckon it should be intuitive for users since a non filtered query searches "everything" and a specific filter looks at specific fields.

@martenson
Copy link
Member

the improvements in this PR are good, I am not a fan of disabling the main search field but other than that I am ready to merge this. Thank you or working on it.


My other feedback is more conceptual and not 24.0-bound. I believe in grids there should be one search bar for a given set of objects. It should be powerful, have shortcuts for filters, have advanced syntax for pro users, but in the end there should be only one place where to type and the majority of real estate should be taken by results.

Screenshot 2024-03-20 at 1 50 33 PM

@ahmedhamidawan
Copy link
Member Author

I am not a fan of disabling the main search field

Not disabling it anymore, and fixing the selenium fails...

It groups `is` type boolean filters on one line, and in checkboxes.
Adding filters applies them immediately, and there is no search button.
@mvdbeek mvdbeek merged commit ea1e84d into galaxyproject:release_24.0 Mar 21, 2024
50 of 52 checks passed
@mvdbeek
Copy link
Member

mvdbeek commented Mar 21, 2024

Thanks @ahmedhamidawan @martenson!

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

Successfully merging this pull request may close these issues.

None yet

3 participants