Refactor address form component (properly this time)#6225
Refactor address form component (properly this time)#6225tvdeyen merged 5 commits intosolidusio:mainfrom
Conversation
6056a68 to
c3011c9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6225 +/- ##
==========================================
+ Coverage 86.53% 88.75% +2.22%
==========================================
Files 520 849 +329
Lines 11947 18326 +6379
==========================================
+ Hits 10338 16265 +5927
- Misses 1609 2061 +452 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tvdeyen
left a comment
There was a problem hiding this comment.
Love it. I have one note about easy customization of this component
There was a problem hiding this comment.
I think if we make these two constants private methods one could inherit from this component and override this in order to customize its one address form more easily.
There was a problem hiding this comment.
I'd love for us to favour composition-based extension approaches rather than inheritance-based ones. I know that SolidusAdmin is pre-1.0, but I've already seen the pain of changes to components breaking my subclasses. I think in this case we should not provide the subclass option and just let people wrap the class if they want.
|
Agreed. I just don't think constants are the right way to store sets of fields. Do you have an example where constants help with composition?
|
|
I don't think it necessarily encourages composition, but I would say it encourages subclassing. If we want to make that functionality controllable, perhaps a different approach would be better... but I'm not sure exactly how that would all fit together. |
|
hi guys! If by customizing you mean allowing to include completely custom fields along with the default ones provided (or skip defaults at all), it's possible to do so without the need to subclass/override current component, I'll just need to make a few changes to the component. |
39c87f1 to
bc7ef6a
Compare
bc7ef6a to
35dd1f5
Compare
Break form into View Component slots, with more granular rendering control of each field. Form composition is decided at initialization, where user can provide `fields_preset` option (:contact by default) which then will pick relevant fields for given form. I.e. fields :name, :vat_id, :reverse_charge_status are relevant only for "contact" address type (e.g. customers, companies), while not relevant for "location" address type (e.g. stock locations). Additionally, user can specify further which fields to include or exclude for even more customization, e.g. stock location model does not have `email` attribute yet, if we were to release new stock location form first and then migrate model to add `email` column, we can do that by specifying `exclude_columns: :email` when rendering stock location form and remove it when migration is in place.
An updated iteration of address component: - now using default slots (https://viewcomponent.org/guide/slots.html#default_slot_name) and allow overriding them; Note: `SlotableDefault` had to be patched as it has a bug: ViewComponent/view_component#2169 - default address fields are contained in separate components (currently "contact" and "location" with a set of relevant fields for both); - it is possible to add a new default named fieldset by creating new fieldset component and placing it along with two other default ones; - if user wants to include more fields to the default fieldset or redefine a default field, they can pass extension definitions in `extends` parameter; - to opt out of rendering certain fields one can pass `excludes` array, and those fields will not be included in the view; - users can pass their own fieldsets if default ones do not fit their needs, defining fieldset slot `#with_fieldset &block` and `block` evaluated content will render instead of a default fieldset;
35dd1f5 to
6d096ba
Compare
|
@tvdeyen @jarednorman
Some examplesDefault fieldset "contact"
Default fieldset "location"
With extended fields
With custom fieldset
|
tvdeyen
left a comment
There was a problem hiding this comment.
Nice idea. I need to play around with it a bit more but it looks like a great way of adding address components. I have some nits about placing the view component extension module.
There was a problem hiding this comment.
if we move the file somewhere into app/** it would be auto loaded by Rails's autoloader.
Since this is a module anyway, why not put it into app/components/concerns to match Rails' default module paths?
There was a problem hiding this comment.
the reason why I decided not to put it into components concerns folder is that it's a monkey-patch of ViewComponent::SlotableDefault and I usually like to place monkey-patches/extensions separately from other classes/modules (hence the ext folder), how about we place it in app/lib/ext? so that it is still separate and we make use of Rails autoload
There was a problem hiding this comment.
I understood that this is a monkey patch. There is already a place in solidus (app/decorators) for them, but since you explicitly include this anyway, we can simply use the concerns folder. I order to not forget about this file it is good practice to build a version guard into your monkey patch
if Gem::Version.new("3.22.0") < Gem::Version.new(ViewComponent::Version)
raise "remove me"
endThere was a problem hiding this comment.
And FYI: We will migrate from decorators to patches soonish https://github.com/friendlycart/flickwerk
There was a problem hiding this comment.
good to know thanks!
just out of interest, I can't find app/decorators folder anywhere
There was a problem hiding this comment.
Not in core, true. But all extensions and many apps use this pattern.
Thats also a reason why I dont want to introduce it and make use of the concerns folder.
admin/lib/solidus_admin/engine.rb
Outdated
There was a problem hiding this comment.
This seems to be another way of loading concerns/monkeypatches. I am not sure we need that if we make use of Rails autoloading
There was a problem hiding this comment.
I am not a huge friend of having surprising behaviour like this in a template. We could make use of the render? callback in the component class instead.
Also uses an updated solution from ViewComponent (the issue has been resolved but will only be released in v4), also adds a guard clause to remove the patch once we have upgraded ViewComponent to `>= 4`
|
@tvdeyen I've addressed your comments, and, as I mentioned in one of the commits, the issue with ViewComponent default_slot has been fixed, but it won't be around until v4, so the patch is now using an updated solution from their PR and a guard has been added to remove the patch once we have upgraded VC to v4 |




Unblocks #6160
Summary
With the release of #6168 (addition of new fields to address form) previous updates to the address form component (#6191) were not enough for stock locations form in #6160 to work correctly, so, in order to fix that, this PR introduces fields composition to the address form component.
Break form into View Component slots, with more granular rendering control of each field. Form composition is decided at initialization, where user can provide
fields_presetoption (:contact by default) which then will pick relevant fields for given form. I.e. fields :name, :vat_id, :reverse_charge_status are relevant only for "contact" address type (e.g. customers, companies), while not relevant for "location" address type (e.g. stock locations).Additionally, user can specify further which fields to include or exclude for even more customization, e.g. stock location model does not have
emailattribute yet, if we were to release new stock location form first and then migrate model to addemailcolumn, we can do that by specifyingexclude_columns: :emailwhen rendering stock location form and remove it when migration is in place.Showcase
Screen.Recording.2025-04-30.at.22.40.09.mov
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: