Improve form f.inputs attributes rendering #8439
Conversation
86aa14a to
95996d4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8439 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 141 141
Lines 4055 4060 +5
=======================================
+ Hits 4019 4024 +5
Misses 36 36 ☔ View full report in Codecov by Sentry. |
| @opening_tag = "<fieldset #{fieldset_attrs}>#{legend_tag}<ol>" | ||
| @closing_tag = "</ol></fieldset>" |
There was a problem hiding this comment.
Note that it's not so trivial to use rails helpers to build the fieldset and ol since they are fragments.
| private | ||
|
|
||
| def tag_attributes(html_options) | ||
| # When support for Rails 6.x is dropped, use tag.attributes instead. |
There was a problem hiding this comment.
Is there a standard way to mark code to be reconsidered after rails upgrades?
| def tag_attributes(html_options) | ||
| # When support for Rails 6.x is dropped, use tag.attributes instead. | ||
| # | ||
| # tag.attributes html_options | ||
| # | ||
| tag.tag_options(html_options.to_h).to_s.strip.html_safe | ||
| end |
There was a problem hiding this comment.
I understand that this is done to support both Rails 6.1 and 7+ but we want to use a conditional, so if it's Rails 6.1 we use the tag.tag_options method, otherwise always use the current tag.attributes method. I would expect the former to be removed from Rails at some point in the future so that's why I want to make sure we isolate it to only those still running Rails 6.1 which eventually we'll drop support for.
There was a problem hiding this comment.
I couldn't find any examples of checking the rails version in the ActiveAdmin codebase. Would you prefer feature detection or a version check?
For example, something like this?
# Rails.version is a String, and both "6.1" <=> "7" and "7.1" <=> "7" work as expected
if Rails.version < "7"or this?
if tag.respond_to?(:attributes)There was a problem hiding this comment.
@javierjulio I went with the latter approach, but am happy to revisit.
There was a problem hiding this comment.
I would use if Rails::VERSION::MAJOR <= 6 to isolate the old method and that way the else condition is for the current approach. I would be ok with feature detection but favor the version check in this case because otherwise we'd be falling back to the old approach.
There was a problem hiding this comment.
Thanks! Feature detection didn't work anyway 🤦🏼♂️
dfd45c3 to
c5b1536
Compare
|
Is it possible to re-run CI? This looks like a transient error:
|
|
@amiel GitHub Actions has been reporting issues for the past hour and was just resolved a few minutes ago. Either way looks like latest build is green so it's ok. |
|
Thanks @javierjulio. I think this PR is ready to go. I appreciate you taking the time to help me through this change. It will clean up a bunch of code at Bento. Would you consider another PR based on the |
but use tag.attributes if available
c5b1536 to
4312e2b
Compare
|
@amiel you're welcome. I will not do a backport release but @mgrunberg has done those previously. Since he engaged I believe he would consider a release if you open a PR based on the |
* Improve rendering attributes for f.inputs * Support rails 6.1 but use tag.attributes if available * Use tag.legend instead of "<legend ..."
|
@javierjulio, @amiel I have some free time today. I'm going to backport this and release it. @amiel, don't worry about the backport PR. |
* Improve rendering attributes for f.inputs * Support rails 6.1 but use tag.attributes if available * Use tag.legend instead of "<legend ..."
#8439 improved form attributes rendering but introduced an issue if host app has an admin for Tag resource. The problem is that `tag` (`tag.attributes`) refences the resource and not the `TagHelper`. Let's prefix the call with `helpers`.
#8439 improved form attributes rendering but introduced an issue if host app has an admin for Tag resource. The problem is that `tag` (`tag.attributes`) refences the resource and not the `TagHelper`. Let's prefix the call with `helpers`.
be able to customize Tag resource form #8439 improved form attributes rendering but introduced an issue if host app has an admin for Tag resource. The problem is that `tag` (`tag.attributes`) refences the resource and not the `TagHelper`. Let's prefix the call with `helpers`.
* Improve form f.inputs attributes rendering (#8439) * Improve rendering attributes for f.inputs * Support rails 6.1 but use tag.attributes if available * Use tag.legend instead of "<legend ..." * use helpers to prevent calling resource methods (tag admin) * be able to customize Tag resource form #8439 improved form attributes rendering but introduced an issue if host app has an admin for Tag resource. The problem is that `tag` (`tag.attributes`) refences the resource and not the `TagHelper`. Let's prefix the call with `helpers`. --------- Co-authored-by: Amiel Martin <amiel.martin@gmail.com>
This resolves #8436 by using standard rails helpers to build html tags and attributes.
Note that this would be easy to backport to v3 and I'm happy to open another PR if a backported release would be considered.