administrate
administrate copied to clipboard
Introduce partial search similar to Rails
Introduced partial search similar to Rails, as an alternative to to_partial_path.
Models that have an inheritance relationship often use similar or almost identical partials. Being able to reuse partials not only makes it easier to create new fields, but also improves maintainability.
I tried deleting the HasManyVariant partial as a test, and it continues to operate normally using the HasMany partial. Pleach check customers.log_entries field. http://localhost:3000/admin/customers http://localhost:3000/admin/customers/4
Please review.
refs https://github.com/rails/rails/blob/d39db5d1891f7509cde2efc425c9d69bbb77e670/actionview/lib/action_view/view_paths.rb#L23-L29 https://github.com/rails/rails/blob/d39db5d1891f7509cde2efc425c9d69bbb77e670/actionview/lib/action_view/view_paths.rb#L72-L77
Thank you for checking. I'm not sure if this will help with #2049. It seems like solving 2049 might require implementing a new feature, such as an option to specify a custom partial.
For now, I’ve rebased on the latest main and checked it with standardrb.
I'll address the feedback separately. Thanks again!
rebased again
Thank you again. I have been playing with it today and have pushed some changes to my fork: https://github.com/thoughtbot/administrate/compare/main...pablobm:administrate:feature/inherit_partial. What do you think?
Some comments:
Re: if superclass == Administrate::Field::Base - I think it's fine if fields/base is included. Your solution includes fields/associative in associative fields, which isn't ideal either. However both are filtered out by lookup_context.template_exists?, so no harm is done.
We could do something like the abstract? you propose, but I think it's not necessary.
Re: _partial_prefixes - I'm not sure about its pseudo-private nature. In the end, it's a substitute for to_partial_path, so I think we should accept it openly and call it partial_prefixes (no underscore suffix).
I was also wondering if we need some sort of back-compatibility for users or plugins who implement to_partial_path... but I'm not sure this is needed. If we find that this is a problem for someone, we can add a compatibility layer with a deprecation notice.
Thank you for your review. I fully agreed with your suggestions and have merged the changes.
Re: if superclass == Administrate::Field::Base -
Understood. I also came up with a use case for field/base, so I think this approach is fine.
We could do something like the abstract? you propose, but I think it's not necessary.
OK
Re: _partial_prefixes -
I was also using this by copying it from the Rails source without fully understanding it. Since it doesn't seem to fit the conventions of this gem, I think it's fine to remove the prefix. My apologies.
to_partial_path
I've seen some plugins customize this in the past, often in cases where a Field class inheriting from HasMany wanted to reuse the original HasMany view. Since that is exactly what we are trying to achieve with this feature, I don't think it's a problem.
https://github.com/XPBytes/administrate-field-scoped_has_many/blob/master/lib/administrate/field/scoped_has_many.rb#L17
Sorry about that. I reviewed it again and noticed something, so I added another commit.
local_partial_prefixes should be kept because it needs to be customizable for each Field.
For example, it would be necessary when introducing looks.
Please take a look.
Good point 🙂 One last thing: I have removed the private_class_method declaration, as we don't do that anywhere in the codebase. I prefer not to introduce new idioms unless we have a good reason.
I took the liberty of pushing the change to your branch. What do you think?
I took the liberty of pushing the change to your branch.
No problem. Thank you as always!
Merging! Thank you, twice: for your contribution and for your patience :medal_sports: