Make filters into a simple form #136
Conversation
| def filter_hidden_field_tag(key, value) | ||
| is_collection = ->(key) { !Filter::Params::PERMITTED_PARAMS.include?(key.to_sym) } | ||
| name = is_collection.(key) ? "#{key}[]" : key | ||
| hidden_field_tag name, value, id: nil | ||
| end |
kevinmcconnell
left a comment
There was a problem hiding this comment.
Nice to see how simple this turned out to be. I realize there's a few details left to tidy up on the form, but the overall approach here looks great to me.
| <%= check_box_tag "assignee_ids[]", Current.user.id, filter.assignees.include?(Current.user), id: "assignee_ids_me", hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignments" } %> | ||
| <%= label_tag "assignee_ids_me", "Me", class: "btn btn--plain filter__button" %> |
There was a problem hiding this comment.
Could consider adding the "me" treatment client-side, so that the page remains cacheable for all users. Would be particularly nice if the text/position could be done in CSS alone (and then use a generic "me" controller to add that class). But if we need more than CSS, could do it directly in JS.
(I realize you're still working on this, so maybe already have something planned for this part).
There was a problem hiding this comment.
Yes! It's something I'm considering, but I paused to think more about buckets, which is the only part of this form where we shouldn't render the same options for everyone (some buckets may be confidential). Maybe we could cache everything but the buckets section.
I know we just talked about transitioning into shipping the shaped feature as a whole — was just a bit eager for everyone to use this version since we've gone through multiple designs 😅
| <% Current.account.users.active.without(Current.user).order(:name).each do |user| %> | ||
| <li> | ||
| <%= button_to_chip user.name, params: { text: "for #{user.name}", name: "assignee_ids[]", value: user.id, frame: :assignee_chips }, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignments" } %> | ||
| <%= check_box_tag "assignee_ids[]", user.id, filter.assignees.include?(user), id: "assignee_ids_#{user.id}", hidden: true, data: { action: "filter-form#clearCategory", filter_form_name_param: "assignments" } %> | ||
| <%= label_tag "assignee_ids_#{user.id}", user.name, class: "btn btn--plain filter__button" %> | ||
| </li> | ||
| <% end %> |
There was a problem hiding this comment.
Really like how simple these sections become now. It's all just very vanilla form controls built off the models. We could maybe move the sections to partials or helpers if we want to. But in any case, there's very little indirection here, which makes the workings very obvious.
| #showChip(button) { | ||
| button.querySelector("input").disabled = false | ||
| button.hidden = false | ||
| } |
There was a problem hiding this comment.
Guessing we don't need this any more, since the selected filter chips are visible on page load? We'd just need the "hide" side of it?
There was a problem hiding this comment.
This is something I need to revisit. If we remove the button from the markup, the auto-submit stimulus action doesn't happen. So we disable the input instead.
But then Turbo caches the disabled/hidden input! So we use this to display it once more when navigating back. I think we should be able to make this a non-issue. And if we can't maybe we can call this on turbo:before-cache or on turbo:load to communicate the purpose.
There was a problem hiding this comment.
Clicking a chip effectively takes you to a version of the page with only that filter condition removed. And the filters are all query params. So could the chips simply be links?
Then there’d be no JS involved at all. Just a series of links rendered by the server, where each one has a url with all the other filter params except its own. And navigation should work as expected.
There was a problem hiding this comment.
Yeah I was shooting for the immediate disappearance of the button after clicking which is pretty nice UX. But we can try using links and see how it feels in practice!
EDIT: Ah, but we can just remove the element with a super lightweight controller after click and still let the link do its thing. Yeah, I'll give that a try.
There was a problem hiding this comment.
we can just remove the element with a super lightweight controller after click and still let the link do its thing
Agreed, I think that's the sort of thing we should try. In general, I find it usually works out well to go with the simpler design and just see how the UX feels. The fewer moving parts the better (usually), and often it's actually fine as-is. But if needed we can address the look & feel separately on top of it, without making the underlying design change to accommodate it.
Also, removing the chip is one way to go, but we may also be able to style the :active pseudo selector in a way that makes clicking it feel good too.
There was a problem hiding this comment.
Also, changing it to a link means Turbo can preload it in hover, so it might already be about as fast as what you could do in JS.
There was a problem hiding this comment.
Right, that same line of thinking led to simplifying the filter form's design, too. I'll err on the side of web standards on the next features 👍 and shipping the whole thing together as well when possible.
Quick spike so we can try out the new design in #131 (comment)
You can still persist search terms if you use the original search bar we never removed, and then open the filters dialog and "save to home screen". I wired it up so we can test things out, but we might want to remove the persistence part of it.
cc @jzimdars @kevinmcconnell