pantographe / view_component-form

Rails FormBuilder for ViewComponent
MIT License
204 stars 16 forks source link

Example with element_proc option #142

Closed wooly closed 1 year ago

Spone commented 1 year ago

We'll take care of updating the README.

wooly commented 1 year ago

We suggest a small naming change.

Also, can you implement the same changes in CollectionRadioButtonsComponent?

Yep, can do that in a few hours :+1:

wooly commented 1 year ago

@Spone how does that look?

Spone commented 1 year ago

@wooly looking good! We'll update the README and merge.

wooly commented 1 year ago

Many thanks!

rubiii commented 1 year ago

Hey. We started using this library a couple of weeks ago and this feature would be greatly appreciated. So thank you very much!

We would suggest a small change though. Currently, the element_proc is expected to be passed via
options and that's fine, as long as you don't also have any html_options in your template.
This is how we would like this to work if possible:

<%= f.collection_check_boxes :options, options, :id, :name, {}, disabled: disabled?, element_proc: ->(b) do %>

Since both args are optional, we fixed this on our side by just looking into both hashes:

element_proc = options.delete(:element_proc) || html_options.delete(:element_proc)

What do you think?

Spone commented 1 year ago

Good call @rubiii! Can you make the change @wooly please?

rubiii commented 1 year ago

@wooly I could prepare a PR to your branch if that would help you out. Given my proposed changed looks valid to you of course.

wooly commented 1 year ago

@rubiii that would be super if you could do that as I'm currently on holiday! Thanks! I guess what we're saying then is that the element_proc can be passed in via both options and html_options? Should we then delete from both in the extremely unlikely case that it's specified in both?

rubiii commented 1 year ago

Thanks for replying from your holidays @wooly :)

Good question. We could check and delete the option from both hashes and also raise an error to cover the probably unlikely case where both option hashes include an element_proc. That would be a little more code, but it's probably good to prevent any confusion.

Spone commented 1 year ago

That would be a little more code, but it's probably good to prevent any confusion.

Agreed!

Spone commented 1 year ago

@rubiii @wooly let us know if you need help to wrap this up!

wooly commented 1 year ago

Sorry for the delay. Have merged @rubiii's PR into my branch now, so we should be good to go.

Spone commented 1 year ago

Hmm we have to fix the CI failures on main first :/

wooly commented 1 year ago

@Spone anything I can do to get this over the line?

Spone commented 1 year ago

Hi @wooly thanks for your time on this PR. The past few months have been pretty hectic at work and now I'm on holiday to recharge batteries 🙂

I think we still have to figure out why CI is failing on main before releasing a new version, but in the meantime I think we can go ahead and merge this, if you can update the changelog beforehand it would be perfect 🙏

wooly commented 1 year ago

Thanks @Spone - have updated CHANGELOG.md