Add tbody_html and row_html options to TableFor and IndexAsTable #8423
Conversation
0340409 to
7785815
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8423 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 141 141
Lines 4060 4064 +4
=======================================
+ Hits 4024 4028 +4
Misses 36 36 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@thibaudgg thanks for finding the time to submit this. I'm open to suggestions on the new argument name but this has to be generic, not a "data" attributes specific key.
| @tbody = tbody data: @tbody_data do | ||
| # Build enough rows for our collection | ||
| @collection.each do |elem| | ||
| tr(id: dom_id_for(elem), class: @row_class&.call(elem)) | ||
| tr(id: dom_id_for(elem), class: @row_class&.call(elem), data: @row_data&.call(elem)) |
There was a problem hiding this comment.
I don't want to introduce "data" based keys. We should have an "html" or "html_options" based key otherwise this can only be used for data attributes and we'll be back here when needing to add support for other attributes. I'm open for suggestions on how to name it but I don't want these options being "data" specific. Something like tbody_html or tbody_html_options so other attributes can be set.
There was a problem hiding this comment.
Good point, I switched to the tbody_html options, as I found more used of *_html than *_html_options in the codebase.
For the row_data it's a bit more complex as we need the proc on the row element. We could add a new row_html method with an optional proc arguments, but we would also need to still support the row_class for backward compatibility. Which I guess would overwrite a class set by row_html if both options are used? What would you prefer?
There was a problem hiding this comment.
If you prefer to keep row_class support, I think it would be fine if it's present to reverse merge that into the new row_html value so the class in row_html would take precedence over row_class. We want to phase out row_class and for setting whichever html options desired. Thank you.
There was a problem hiding this comment.
Added row_html, but kept row_class for backward compatibility. I think we should be good now.
7785815 to
0d33879
Compare
0d33879 to
5bd07c1
Compare
Use my branche until the PR is merged and release. activeadmin/activeadmin#8423
7eca533 to
e41e0a1
Compare
There was a problem hiding this comment.
@thibaudgg I just have documentation updates, everything else looks great. We'll leave in row_class support for now and handle a proper deprecation separately. My main concern is just getting in the options to handle any attribute as you've added here. Thank you again for tackling this.
This patch adds a `tbody_html` and `row_html` options to the `TableFor` (and `IndexAsTable`) classes. These options allow you to pass a hash of html options attributes to the `tbody` and `tr` elements. This is useful when you want to add features to the table that require data attributes. For example, adding a sortable row via JavaScript. You can see how I'm currently using this approach in [this app](https://github.com/acp-admin/acp-admin): - [monkey patching](https://github.com/acp-admin/acp-admin/blob/master/config/initializers/active_admin.rb#L261-L328) (same approach as this PR) - [using both options on a `table_for` method](https://github.com/acp-admin/acp-admin/blob/master/app/views/active_admin/deliveries/_baskets.html.arb#L10-L13) for using [Stimulus Sortable component](https://www.stimulus-components.com/docs/stimulus-sortable/). The existing `row_class` option is kept for backwards compatibility, but the `row_html` will take precedence if both are provided.
e41e0a1 to
a56a926
Compare
|
@javierjulio great, thanks for the review. I just updated the PR with your changes. 👍🏻 |
My PR has been merged 🎉 activeadmin/activeadmin#8423
This patch adds a
tbody_htmlandrow_htmloptions to theTableFor(andIndexAsTable) classes. These options allow you to pass a hash of HTML optionsattributes to the
tbodyandtrelements.This is useful when you want to add features to the table that require data
attributes. For example, adding a sortable row via JavaScript.
You can see how I'm currently using this approach in this app:
table_formethod for using Stimulus Sortable component.The existing
row_classoption is kept for backwards compatibility, but therow_htmlwill take precedence if both are provided.