Skip to content

Introduce partial search similar to Rails#2467

Merged
pablobm merged 5 commits intothoughtbot:mainfrom
goosys:feature/inherit_partial
Feb 25, 2025
Merged

Introduce partial search similar to Rails#2467
pablobm merged 5 commits intothoughtbot:mainfrom
goosys:feature/inherit_partial

Conversation

@goosys
Copy link
Copy Markdown
Contributor

@goosys goosys commented Dec 7, 2023

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

@goosys goosys force-pushed the feature/inherit_partial branch from 577a77d to 1fe7873 Compare December 26, 2023 07:11
Copy link
Copy Markdown
Collaborator

@pablobm pablobm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw this for the first time and it caught my eye. Let's come back to it after the PRs we are working on now. Perhaps it can also help solving #2049?

@goosys goosys force-pushed the feature/inherit_partial branch from 1fe7873 to 2084605 Compare October 4, 2024 16:41
@goosys
Copy link
Copy Markdown
Contributor Author

goosys commented Oct 4, 2024

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!

@goosys goosys force-pushed the feature/inherit_partial branch from 2084605 to 5027f67 Compare October 8, 2024 12:32
@goosys goosys force-pushed the feature/inherit_partial branch from 5027f67 to d756627 Compare February 20, 2025 07:17
@goosys
Copy link
Copy Markdown
Contributor Author

goosys commented Feb 20, 2025

rebased again

@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Feb 21, 2025

Thank you again. I have been playing with it today and have pushed some changes to my fork: 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.

@goosys
Copy link
Copy Markdown
Contributor Author

goosys commented Feb 22, 2025

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

@goosys
Copy link
Copy Markdown
Contributor Author

goosys commented Feb 22, 2025

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.

@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Feb 23, 2025

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?

@goosys
Copy link
Copy Markdown
Contributor Author

goosys commented Feb 23, 2025

I took the liberty of pushing the change to your branch.

No problem. Thank you as always!

@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Feb 25, 2025

Merging! Thank you, twice: for your contribution and for your patience 🏅

@pablobm pablobm merged commit 8b23aba into thoughtbot:main Feb 25, 2025
pablobm added a commit that referenced this pull request May 2, 2025
There is an inconsistency between the `administrate:field` generator and the
custom field `HasManyVariant` present in the demo app. They use different
conventions for naming and file paths.

After thinking about which of the two styles is right or wrong, I think the
generator is doing the right thing. This PR moves `HasManyVariant` and
renames it `HasManyVariantField`, in order to follow the convention set out
by the generator (which is also the one shown in the docs).

Notes:
- The field class is generated under `app/fields`. The location means it's
  autoloaded and we don't need a `require`.
- The `Field` suffix may seem annoying, but it's not different from
  controllers, helpers, etc, and removes the need for a namespace
  (eg: `Field::HasManyVariant`).
- This establishes a difference between the official field types
  (eg: `Administrate::Field::String`) and custom ones (eg:
  `MyStringField`), but I think this is fine.

Tangentially I also noticed that, if for any reason a user puts the template
files in the wrong place, this is difficult to debug as Administrate will
just fail silently, rendering empty templates. This is due to the changes
introduced at #2467. This
should not be an issue if using the generator, but might be if creating the
field manually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants