Refactor DeveloperQuery #821
Conversation
soumyashree25
force-pushed
the
developer-query-refactor
branch
from
2c62937
to
380484a
Compare
|
Hi @joemasilotti , I have extracted sort and countries filter from |
| def initialize(options = {}) | ||
| attr_reader :query_item_builder, :query_items | ||
|
|
||
| def initialize(options = {}, query_item_builder = Developers::QueryItemBuilder.new) |
There was a problem hiding this comment.
Currently options are being provided to support other existing query items.
Once I extract all other query items, we will only pass query_item_builder object.
| add_sorting_query | ||
| add_countries_filter_query |
There was a problem hiding this comment.
I think that these two should be passed in as parameters to initialize. That way we can pass in any combination we want - and future queries only require changes to the new query item and not this class.
| QUERY_ITEM_OPTIONS.each do |method_initial, _| | ||
| define_method "#{method_initial}_query_item" do | ||
| query_items.detect { |query_item| query_item.type.has_key? method_initial } | ||
| end | ||
| end |
There was a problem hiding this comment.
This feels like it is breaking the point of the refactor. If the public interface exposes the different filters by name then anyone using the query needs to know about reach filter.
There was a problem hiding this comment.
@joemasilotti since methods like sort, countries etc are called on developer query object in many places (example: https://github.com/joemasilotti/railsdevs.com/pull/821/files#diff-9b36d228956c34411c77b9a8f1fc0f9831f453e3c4fb52b050673af85a6ee584R14) , somehow we have to expose those either in developer query or query builder.
Any thoughts on how to achieve that if we don't get access to sort_query_item or countries_query_item ?
There was a problem hiding this comment.
One approach is for the query to expose an array of query items publicly. Then, the view can iterate over those to generate the necessary form elements.
The query item can indicate what type of field it requires (text box, select, checkbox, etc.) along with the copy and notes needed for the UI. It can also expose a method that decides if a collapse should be expanded or not.
Does that make sense?
There was a problem hiding this comment.
One approach is for the query to expose an array of query items publicly. Then, the view can iterate over those to generate the necessary form elements.
@joemasilotti made adjustments to countries filter and sort query items, let me know if this is fine.
The query item can indicate what type of field it requires (text box, select, checkbox, etc.) along with the copy and notes needed for the UI. It can also expose a method that decides if a collapse should be expanded or not.
@joemasilotti can you please explain a bit more what do you mean by indicating tags like checkbox, select, etc ?
Should we let the query item return its own html and then embed it to app/components/developers/query_component.html.erb ?
There was a problem hiding this comment.
Thanks for the update!
In an ideal world we could pass in a bunch of QueryItem duck types and a single SortItem into DeveloperQuery and that would be that. The HTML could be generated based entirely by iterating over these items and inferring the type.
But the more I think about it the more I'm wondering if a) I'm overcomplicating it and b) we are rebuilding ransack.
I appreciate the work you put into this PR. But I think I'd like to explore ransack next. I'd still love to award a bounty - send me an email and we can figure it out.
soumyashree25
force-pushed
the
developer-query-refactor
branch
from
380484a
to
da09c31
Compare
closes #805
Pull request checklist
bin/check