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

UI filter builder #3515

Merged
merged 23 commits into from
Mar 16, 2023
Merged

Conversation

WithoutPants
Copy link
Collaborator

@WithoutPants WithoutPants commented Mar 8, 2023

Replaces the add filter dialog with an edit filter dialog. This allows adding/editing filter criterion all at once:

image

  • text fields have a confirm button to modify the value
  • current filter is shown at the bottom, with an extra option to clear all criteria if there is more than two criteria
  • criteria with current values have an x next to their entries which allow deleting the criterion
  • criteria with options are presented as a list of checkboxes

image
image

Mobile view presents the criteria list only, then shifts to the criterion editor when a criterion is selected, with a back button to return to the criteria list.

image

Also adds the criteria count as a pill to the filter button, and adds the Clear All button to the main view as well.

Closes #949

@WithoutPants WithoutPants added improvement Something needed tweaking. ui Issues related to UI needs testing Pull requests that require testing labels Mar 8, 2023
@WithoutPants WithoutPants added this to the Version 0.20.0 milestone Mar 8, 2023
@cj12312021
Copy link
Collaborator

I encountered this bug that wouldn't allow me to specify another filter. Anytime I used the drop down it would send me back to the performers filter.
https://user-images.githubusercontent.com/72030708/223875732-e8604af8-5a75-4d3a-80f8-137a830dbe34.mp4

I also noticed that the submit button is inconsistently placed on a few of the filter menus. It's not clear if this was intentional:
Screenshot 2023-03-08 170251
Screenshot 2023-03-08 170316

@WithoutPants
Copy link
Collaborator Author

Criterion selection bug should be fixed. Thanks for the report.

Yeah, there's a style discrepancy there currently. I wanted to add the confirm button to the input control, but this breaks down when you have a between modifier and need two fields.

I can move it out into a separate button to be consistent, but I want to confirm that this is the best approach from a UX point of view.

The alternatives are:

  • update on keystroke - this is kind of a waste of rendering, especially when rapidly typing into a field. We could debounce it slightly I guess
  • update on blur - the problem with this approach is that it's not intuitive to the user how to confirm a value. They will type in a value, not see any confirm button, and wonder why the filter hasn't updated.
  • reposition the confirm button somewhere else?

I chose this solution based on playing around with the ebay filter UI.

@cj12312021
Copy link
Collaborator

cj12312021 commented Mar 10, 2023

Sorry, I forgot to get back to this. That makes sense. Yeah, I was leaning toward having the button on the input field, but I see that challenge with that when dealing with the between modifiers. I think consistent placement would be best, so the user always knows where to find the button without much thought. So perhaps the button could just stay below the input fields.

I also confirmed that your latest push addresses the issue I mentioned earlier.

@cj12312021
Copy link
Collaborator

I wonder if a design like this might make more sense. The current design leaves a lot of unused space:
Screenshot 2023-03-10 170902

@WithoutPants
Copy link
Collaborator Author

I agree, it looks better imo, and is more consistent with the mobile view:

image

The applicable element is scrolled into view when clicking on a criterion from outside the filter dialog. This behaviour is not triggered when clicking on the pill from inside the dialog, since I couldn't get the scrolling function to consistently scroll to the correct position.

@xx790
Copy link

xx790 commented Mar 12, 2023

Will this UI work well when #2692 gets addressed? (We need multiple filters on the same property like tags)

@cj12312021
Copy link
Collaborator

I agree, it looks better imo, and is more consistent with the mobile view:

image

The applicable element is scrolled into view when clicking on a criterion from outside the filter dialog. This behaviour is not triggered when clicking on the pill from inside the dialog, since I couldn't get the scrolling function to consistently scroll to the correct position.

This looks a lot better and functions as I would have expected.

@cj12312021
Copy link
Collaborator

I do think all the selectors could continue to take the full width as they used to. For example, the modifier selector for play count in your screenshot took up a little under 1/3 of the width where it used to take up the full width.
Screenshot 2023-03-12 143426

@WithoutPants
Copy link
Collaborator Author

Will this UI work well when #2692 gets addressed? (We need multiple filters on the same property like tags)

I don't think it will preclude it from working. The UI design can be iterated on as part of the work on that anyway.

@DingDongSoLong4
Copy link
Collaborator

In terms of looks, this is amazing - infinitely better than before.

I do think the confirm button is a bit annoying though, I can definitely see myself forgetting to click it and then losing the filter after clicking Apply. It also looks kinda out of place on its own line, and is basically just an extra unnecessary click imo.

I get the hesitance to update while typing, but do the extra rerenders really make that much of a difference? Even if debouncing the updates is necessary, I think that would be a much nicer experience than having to confirm each field manually.

Three other minor issues I found:

  • For the Stash ID filter, if you enter an endpoint first and then move on to the ID, the endpoint is cleared.
  • For any of the hierarchical inputs (Studios, Performers, etc), if you select a modifier without adding any values, it will still add an empty criterion to the list.
  • For the timestamp filters (Created At, Updated At), the placeholder text uses 'HH-MM', which should be 'HH:MM'. This actually seems to have been an issue before as well. The backend is also returning all scenes here instead of an error for invalid input, which can be fixed separately at some point.

The Stash ID issue actually ended up being fixed as a byproduct of the quick and dirty change I made to get a feel for the dialog without the confirm button (basically just replace all the setValue calls with onValueChanged and get rid of the value state).

And then the last improvement I can think of would be to validate the date/time/duration/etc inputs, but that can definitely be done later in a separate PR.

@WithoutPants
Copy link
Collaborator Author

Thanks for the feedback.

I've removed the confirm buttons, the value is now updated as typed. Stash ID and hierarchical filters should be fixed, and I've fixed the placeholder text on the timestamp filter.

@DingDongSoLong4
Copy link
Collaborator

Lovely, looks great. I'm still seeing the hierarchical issue for Studios and Tags though, the fix seems to have only affected Performers and Movies.

@WithoutPants
Copy link
Collaborator Author

Thanks, the other criteria should be fixed now.

@WithoutPants WithoutPants merged commit 943a6d3 into stashapp:develop Mar 16, 2023
@echo6ix
Copy link
Contributor

echo6ix commented Mar 17, 2023

I'm having an issue where the country results get cropped

  • Custom CSS disabled
  • Chrome v111

image

@echo6ix
Copy link
Contributor

echo6ix commented Mar 17, 2023

First, I think this is a big improvement, and adding a badge counter to the filter button is a great idea. My only feedback regarding that badge counter is the following:

  1. Badge color. Red is used as a color to denote destructive actions or cancellations throughout Stash. A variation of bright blue might be better for this badge. Off the top of my head, the --info color defined in the CSS would work.
  2. Badge placement. Placement seems a little awkward to me here. I think the above (and below) example from Github would work a lot better
    image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Something needed tweaking. needs testing Pull requests that require testing ui Issues related to UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Fix "Add Filter" button's obstruction by drop-down lists
5 participants