Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

johrstrom
Copy link
Contributor

@johrstrom johrstrom commented Nov 1, 2022

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'} ],

Copy link
Contributor

@lcreid lcreid left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It's always great to hear of new ideas that people have to make this gem better.

Would you mind adding a change to the README.md file that would tell a programmer how they would use your feature to achieve something they want to do? I find that helps me understand your use case. Thanks in advance.

@johrstrom
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@lcreid
Copy link
Contributor

lcreid commented Nov 20, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

lcreid commented Nov 23, 2022

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
Copy link
Contributor Author

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
Copy link
Collaborator

donv commented Aug 31, 2023

Hi @johrstrom and @lcreid !

What is the state of this PR now?

@johrstrom
Copy link
Contributor Author

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 donv merged commit deb502a into bootstrap-ruby:main Sep 7, 2023
@donv
Copy link
Collaborator

donv commented Sep 7, 2023

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

@johrstrom johrstrom deleted the data-radio-buttons branch September 7, 2023 14:03
@johrstrom
Copy link
Contributor Author

Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants