-
-
Notifications
You must be signed in to change notification settings - Fork 463
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
5847 surface common filters with helper #5879
5847 surface common filters with helper #5879
Conversation
d7547e6
to
ed6bce2
Compare
ed6bce2
to
9a14fec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty happy with this. I'll manually test it a bit and let Sam provide his input.
@mononoken Do you think you could add something to the system test that uses the exposed filters? ex:
Have drafts > check present
hide drafts > filter > check absent
expand > select another filter > filter
select all filters work > check still expanded by selecting some other expanded filter option
@elasticspoon , it looks like the first case was already tested at :93, but I have added some system tests to test the hide draft filter and the filter menu collapse functionality. If I missed any cases or the tests could be improved, please let me know! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @mononoken! I like this, too, because I think it will be easier to hook into Hotwire when we add it to the project.
<% collapse_class = expand_filters? ? "collapse show" : "collapse" %> | ||
<div class="card-content mb-10 ml-20 <%= collapse_class %>" id="filter-card-body"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you wanted to be fancy you could do:
<div class="<%= token_list("card-content mb-10 ml-20 collapse", {show: expand_filters?}) %>" id="filter-card-body">
https://apidock.com/rails/ActionView/Helpers/TagHelper/token_list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thanks so much.
If you are still interested in turbo I can write up an issue for that π
Thank you both for the feedback on the issue! I'd be happy to work on implementing Turbo on the next issue, @elasticspoon . |
Note: This is an alternative approach to #5871
Instead of persisting client state with sessionStorage, I scrapped the idea of using client state for the collapse logic. Instead, I am using the original method of having the menu shown or hidden based on the existence of the filter params. However, I have removed the "surfaced" params from this check, so that when you filter only by an option such as "Hide draft", it will not open the filter options menu.
What github issue is this PR for, if any?
Resolves #5847
What changed, and why?
Sorted by
andHide drafts
filter options outside of the collapsable area of the form element, so that they display regardless of filter menu collapse status.expand_filters?
to prevent filter menu from expanding when filtering only by "surfaced" filter options.How is this tested? (please write tests!) ππͺ
I added tests for the new
expanded_filters?
helper method.Screenshots please :)
Feelings gif (optional)
Instead of a gif, I asked ChatGPT to write a haiku about Bootstrap.
γγΌγγΉγγ©γγ
εΆη΄γζγγ¦
θͺη±γζ±γ