bootstrap-ruby / bootstrap_form

Official repository of the bootstrap_form gem, a Rails form builder that makes it super easy to create beautiful-looking forms using Bootstrap 5.
MIT License
1.64k stars 351 forks source link

allow for data- attributes to pass through for radio button collections #665

Closed johrstrom closed 10 months ago

johrstrom commented 1 year ago

Hi, I need the capability to add data- HTML attributes to collections of radio buttons. If found that this works, it may or may not be implemented the way you'd like - so I'm happy to fix/change whatever you ask (this PR is editable by maintainers).

Just a little more information here, you can see from the tests I'm adding data-city attributes to each input tag.

<input class="form-check-input" data-city="east" id="user_misc_1" name="user[misc][]" type="checkbox" value="1" />

We pass in Arrays using :first and :second for value and label functions respectively. One element, the Hash, is all the extra HTML data- attributes we'd like to add.

['1', 'Foo', {'data-city': 'east'} ],
johrstrom commented 1 year ago

Would you mind adding a change to the README.md file

Of course! I added it to the collections section as this only applies to collections (IDK if folks can already do this with a single radio_button). I didn't add an image because visually it's the same as before, but I'm happy to if you want one.

johrstrom commented 1 year ago

Is there something wrong with the example and/or would you like other updates?

lcreid commented 1 year ago

Thank you for your patience. I really had to do some updating of personal projects/servers the last little while, and that sucked up all the time I had available for non-paid work. :-(

The reason this is complicated is because, while I like your solution, it may bring to light a bigger issue. I'm guessing that the way to add attributes to the individual check boxes (and radio buttons) in a more general way, if you were using the underlying Rails helper, would be to provide a block. and then you have much more control over the rendering of the individual check boxes, including adding whatever data attributes you needed.

Unfortunately, I already have a big refactoring that I think I need to do to solve another issue, so I can't promise to work on #477 in the next few weeks. So here's what I'd like to propose (and they're not mutually exclusive):

I'd be really happy to see what you think about the second one. We would have to have a discussion about it (i.e. don't run off and implement a whole PR just yet). But I understand if you don't have time to take it on. At least the first one point would allow you to move on.

What do you think, @johrstrom ?

johrstrom commented 1 year ago

Thanks for the response. I'm happy to look into providing blocks. I work in OSS as a part of my job (this if for that job) so I can make the time.

You're the maintainer, and as a fellow maintainer of OSS, I get that you may not want to take on additional features when you know/suspect they'll be replaced. So it's totally fine by me to close this PR and only look into providing blocks. What's more is is that my project is on bootstrap 4, so even if this were merged, I won't update and use it for quite some time. So there's no urgency.

We would have to have a discussion about it (i.e. don't run off and implement a whole PR just yet).

Happy to continue the discussion on #477

johrstrom commented 1 year ago

Looking into #477 and the Rails library, I have to set the expectation here that I doubt I'll get very far in implementing it. And if I do it'd be quite some time. It's not really an issue of time (well it is kinda) it's more of the complexity of the change and my familiarity with this library.

lcreid commented 1 year ago

I hear you. I think what has happened is that the structure of the code meshed well with Bootstrap 3, and worked okay with Bootstrap 4, now we're at Bootstrap 5, and we've been adding features all that time, and the code is getting hard to change. There's another issue that I simply can't fix because I can't find an incremental way to get the code there. And I just don't have the time for a rewrite.

If I manage to do some work, do you mind if I mention you for reviews or with questions? I find other eyes can be really helpful.

johrstrom commented 1 year ago

If I manage to do some work, do you mind if I mention you for reviews or with questions? I find other eyes can be really helpful.

Sure thing! Like I say I'm a maintainer of an OSS package, though through my job. In any case, I have incentive to help you because I need this library too, so I'm very happy to do so.

donv commented 10 months ago

Hi @johrstrom and @lcreid !

What is the state of this PR now?

johrstrom commented 10 months ago

What is the state of this PR now?

I don't know. Reading through the comments it seems the TLDR is it's OK, but didn't get merged because a larger refactor is preferred. I cannot initiate such a refactor, but would be happy to help if I can.

That said, if you close this PR my feelings won't be hurt. If you all want to close this in preference for a larger refactor, that's totally understandable.

donv commented 10 months ago

The change is small and well tested and documented, so I merged it.

johrstrom commented 10 months ago

Thanks!