Skip to content

Extend dom_id and dom_class to accept var-args - #44081

Closed
seanpdoyle wants to merge 1 commit into
rails:mainfrom
seanpdoyle:record-identifier-argument
Closed

Extend dom_id and dom_class to accept var-args#44081
seanpdoyle wants to merge 1 commit into
rails:mainfrom
seanpdoyle:record-identifier-argument

Conversation

@seanpdoyle

@seanpdoyle seanpdoyle commented Jan 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Changes dom_id and dom_class to accept a variable set of prefixes:

dom_class @post, :first_prefix, :second_prefix
  # => "first_prefix_second_prefix_post"
dom_class @post, [:first_prefix, :second_prefix]
  # => "first_prefix_second_prefix_post"
dom_id @post, :first_prefix, :second_prefix
  # => "first_prefix_second_prefix_post_123"
dom_id @post, [:first_prefix, :second_prefix]
  # => "first_prefix_second_prefix_post_123"

These changes align with the FormBuilder#field_id interface, which
accepts multiple suffixes arguments.

@rails-bot rails-bot Bot added the actionview label Jan 5, 2022
@seanpdoyle seanpdoyle mentioned this pull request Jan 5, 2022
Merged
@seanpdoyle
seanpdoyle force-pushed the record-identifier-argument branch from b105e51 to 814b048 Compare January 5, 2022 18:44
p8
p8 reviewed Jan 11, 2022
View reviewed changes
Comment thread actionview/lib/action_view/record_identifier.rb Outdated
p8
p8 reviewed Jan 11, 2022
View reviewed changes

@p8 p8 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @seanpdoyle 👋 , I think this would be a nice addition.

Comment thread actionview/lib/action_view/record_identifier.rb Outdated
p8
p8 reviewed Jan 11, 2022
View reviewed changes
Comment thread actionview/lib/action_view/record_identifier.rb Outdated
Comment thread actionview/lib/action_view/record_identifier.rb Outdated
@p8

p8 commented Jan 11, 2022

Copy link
Copy Markdown
Member

@seanpdoyle Can you squash the commits?

@seanpdoyle
Extend dom_id and dom_class to accept var-args
ed6ce19 
Changes `dom_id` and `dom_class` to accept a variable set of prefixes:

```ruby
dom_class @post, :first_prefix, :second_prefix
  # => "first_prefix_second_prefix_post"
dom_class @post, [:first_prefix, :second_prefix]
  # => "first_prefix_second_prefix_post"
dom_id @post, :first_prefix, :second_prefix
  # => "first_prefix_second_prefix_post_123"
dom_id @post, [:first_prefix, :second_prefix]
  # => "first_prefix_second_prefix_post_123"
```

These changes align with the [FormBuilder#field_id][] interface, which
accepts multiple `suffixes` arguments.

[field_id]: https://edgeapi.rubyonrails.org/classes/ActionView/Helpers/FormBuilder.html#method-i-field_id
@seanpdoyle
seanpdoyle force-pushed the record-identifier-argument branch from 95edbd6 to ed6ce19 Compare January 11, 2022 20:04
@p8 p8 added the ready PRs ready to merge label Jan 13, 2022
@p8

p8 commented Jan 13, 2022

Copy link
Copy Markdown
Member

Thanks @seanpdoyle !

@kaspth

kaspth commented Jan 14, 2022

Copy link
Copy Markdown
Contributor

I think these methods have gone for many years with only a single argument prefix. What's the change or use case that necessitates adding this now?

Is there a suitable workaround that users could add in their apps for the multiple prefix version? E.g. doing the concatenation at the call-sites?

I'm leaning towards closing this as is.

@seanpdoyle seanpdoyle closed this Jan 14, 2022
@seanpdoyle

seanpdoyle commented Jan 14, 2022

Copy link
Copy Markdown
Contributor Author

That's sensible. Closing.

@seanpdoyle seanpdoyle mentioned this pull request Dec 14, 2023
Open
@p8 p8 mentioned this pull request Jan 7, 2024
Closed
4 tasks
@bensheldon

bensheldon commented Jun 17, 2025

Copy link
Copy Markdown
Contributor

What's the change or use case that necessitates adding this now?

I think Turbo-Rails is something relatively new that significantly increases the demand for generating matching identifiers that exist across Views and application code. For example, Turbo Broadcast targets.

Is there a suitable workaround that users could add in their apps for the multiple prefix version? E.g. doing the concatenation at the call-sites?

The main benefit of handling multi-value prefixes inside of ActionView::RecordIdentifier is the implicit JOIN constant, which would otherwise be a magic string, and I've myself seen development friction (potential production bugs) in mismatching _ vs - (which was how I got to this issue, as a "gosh, I can't be the only one experiencing this").

@bensheldon

bensheldon commented Jun 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

@seanpdoyle Here's an idea. if the modification of these methods are objectionable, what do you think of proposing a new method like dom_target(*targets) that doesn't have the baggage of the prefix ordering and instead just concatenates whatever is passed in?

Edit: I'll make a quick PR to propose.

@bensheldon bensheldon mentioned this pull request Jun 17, 2025
Merged
4 tasks
@bensheldon

bensheldon commented Jun 17, 2025

Copy link
Copy Markdown
Contributor

Let's see how it goes: #55204

@kaspth

kaspth commented Jun 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

I wouldn't necessarily consider my comments from 3 years ago to still be valid. I hadn't thought of the case with turbo-rails putting more strain on dom_id and dom_class.

While dom_target is nice, it seems fine to me to extend these since they work in a backwards compatible way. I don't think the extra splat-Array allocation will matter much in practice, though I could be wrong.

That said, I don't have any merge rights on here anymore. So it's up to someone else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@p8 p8 p8 left review comments

Assignees

No one assigned

Labels

actionview ready PRs ready to merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@seanpdoyle @p8 @kaspth @bensheldon