Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add support for conditional values to TagBuilder #37872

Merged

Conversation

@joelhawksley
Copy link
Contributor

joelhawksley commented Dec 3, 2019

This PR adds support for conditional values to TagBuilder,
extracting logic we use in the GitHub application,
inspired by https://github.com/JedWatson/classnames.

It’s common practice to conditionally apply CSS classes
in Rails views. This can lead to messy string interpolation,
often using ternaries:

content_tag(
  "My username",
  class: "always #{'sometimes' if current_user.special?} another"
)

By adding support for hashes to TagBuilder, we can instead write the following:

content_tag(
  "My username",
  class: ["always", "another", { 'sometimes' => current_user.special? }]
)

cc @JedWatson

Adds support for conditional values to TagBuilder,
extracting logic we use in the GitHub application,
inspired by https://github.com/JedWatson/classnames.

It’s common practice to conditionally apply CSS classes
in Rails views. This can lead to messy string interpolation,
often using ternaries:

```ruby
content_tag(
  "My username",
  class: "always #{'sometimes' if current_user.special?} another"
)
```

By adding support for hashes to TagBuilder, we can instead write the following:

```ruby
content_tag(
  "My username",
  class: ["always", "another", { 'sometimes' => current_user.special? }]
)
```

cc @JedWatson
@rails-bot rails-bot bot added the actionview label Dec 3, 2019
joelhawksley added 2 commits Dec 3, 2019
@joelhawksley joelhawksley force-pushed the joelhawksley:content-tag-hash-class-conditional branch from e716a1f to 54f418d Dec 3, 2019
Copy link
Member

tenderlove left a comment

I think this makes a lot of sense for CSS classes. Should it be applied to all options though?

@joelhawksley

This comment has been minimized.

Copy link
Contributor Author

joelhawksley commented Dec 4, 2019

@tenderlove this probably doesn't make much sense for values to arguments besides class 👍

@tenderlove tenderlove merged commit 8c121ce into rails:master Dec 4, 2019
2 checks passed
2 checks passed
build
Details
buildkite/rails Build #65294 passed (1 hour, 37 minutes, 53 seconds)
Details
@joelhawksley joelhawksley deleted the joelhawksley:content-tag-hash-class-conditional branch Dec 4, 2019
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Dec 9, 2019
- rails#37872 introduced a regression and you can't do

  ```html.erb
    hidden_field_tag('token', value: [1, 2, 3])
  ```

  This will result in a `<input type="hidden" value=""`>.

  I chose `hidden_field_tag` and the `value` attribute as an example
  but this issue applies to any tag helper and any attributes.

  I don't know if this PR is the right fix since rails#37872 (comment)
  specifies that the feature should only apply for "class" attribute.
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Dec 9, 2019
- rails#37872 introduced a regression and you can't do

  ```html.erb
    hidden_field_tag('token', value: [1, 2, 3])
  ```

  This will result in a `<input type="hidden" value=""`>.

  I chose `hidden_field_tag` and the `value` attribute as an example
  but this issue applies to any tag helper and any attributes.

  rails#37872 (comment)
  mention that the feature should only apply for "class" attribute.

  This commit fix original intent of rails#37872
Edouard-chin added a commit to Edouard-chin/rails that referenced this pull request Dec 9, 2019
- rails#37872 introduced a regression and you can't do

  ```html.erb
    hidden_field_tag('token', value: [1, 2, 3])
  ```

  This will result in a `<input type="hidden" value=""`>.

  I chose `hidden_field_tag` and the `value` attribute as an example
  but this issue applies to any tag helper and any attributes.

  rails#37872 (comment)
  mention that the feature should only apply for "class" attribute.

  This commit fix original intent of rails#37872
joelhawksley added a commit to joelhawksley/rails that referenced this pull request Dec 9, 2019
As a follow-up to rails#37872,
this change introduces a class_names view helper
to make it easier to conditionally apply class names
in views.

Before:
<div class="<%= item.for_sale? ? 'active' : '' %>">

After:
<div class="<%= class_names(active: item.for_sale?) %>">

We've been using this helper in the GitHub monolith
since 2016.

Co-authored-by: Aaron Patterson <tenderlove@github.com>
joelhawksley added a commit to joelhawksley/rails that referenced this pull request Dec 9, 2019
As a follow-up to rails#37872,
this change introduces a class_names view helper
to make it easier to conditionally apply class names
in views.

Before:
<div class="<%= item.for_sale? ? 'active' : '' %>">

After:
<div class="<%= class_names(active: item.for_sale?) %>">

We've been using this helper in the GitHub monolith
since 2016.

Co-authored-by: Aaron Patterson <tenderlove@github.com>
joelhawksley added a commit to joelhawksley/rails that referenced this pull request Dec 9, 2019
As a follow-up to rails#37872,
this change introduces a class_names view helper
to make it easier to conditionally apply class names
in views.

Before:
<div class="<%= item.for_sale? ? 'active' : '' %>">

After:
<div class="<%= class_names(active: item.for_sale?) %>">

We've been using this helper in the GitHub monolith
since 2016.

Co-authored-by: Aaron Patterson <tenderlove@github.com>
@nashbridges

This comment has been minimized.

Copy link

nashbridges commented Dec 27, 2019

This can lead to messy string interpolation,

For honest comparison with the new version, one could use arrays (that were already supported) + pure Ruby

content_tag(
  "My username",
  class: ["always", "another", ("sometimes" if current_user.special?)]
)

vs

content_tag(
  "My username",
  class: ["always", "another", { 'sometimes' => current_user.special? }]
)

Probably, the hash notation was inspired by JS Classnames library. Because in JS something like

[1, 2, (if (true) { 3 })]

is not only ugly, but also an invalid syntax.

@kenn

This comment has been minimized.

Copy link
Contributor

kenn commented Dec 28, 2019

@nashbridges I guess this API has parity with #37918 in mind.

@joelhawksley

This comment has been minimized.

Copy link
Contributor Author

joelhawksley commented Dec 28, 2019

@nashbridges - @kenn is correct. This PR was meant to enable #37918.

@nashbridges

This comment has been minimized.

Copy link

nashbridges commented Dec 29, 2019

@kenn @joelhawksley I see, thanks guys!

On our project we use slim, so the class_names equivalent would be

.wrapper class=("active" if item.for_sale?)

but yeah, I can see that for ERB users the result is much more cleaner.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.