Add [key]Classes method to better handle multiple CSS classes #344

swanson · 2020-12-08T00:29:30Z

Closes #341

👋

Utility-first CSS is becoming increasingly popular as an alternative to BEM (the convention used in the Handbook). This PR adds the [key]Classes property that maps a space separated list of classes into an array.

This new property can be combined with the spread operator for more elegant classList manipulation when you need to change multiple classes at once.

Here is an adapted example from my production codebase:

// highlight_controller.js
export default class extends Controller {
  static targets = [ "subject" ]
  static classes = [ "pulse" ]

  pulse() {
    this.subjectTarget.classList.add(...this.pulseClasses)
  }
}

<div data-controller="highlight" 
     data-highlight-pulse-class="animate-pulse ring-8 ring-teal-600 ring-offset-8 rounded">

  <button data-action="click->highlight#pulse">
    Show me
  </button>

  <span data-highlight-target="subject">
     ...
  </span>
</div>

Pros:

Cleaner usage with utility CSS (e.g. Tailwind)
${key}Classes plural API mirrors target conventions

Cons:

Inconsistency that fooTargets is mapped by multiple data-x-y-targets attributes, while fooClasses is mapped by space separated string (too much magic?)

I've added some basic test cases and happy to add docs if this is something you'd like added. Open to any and all feedback or alternatives :)

packages/@stimulus/core/src/class_properties.ts

swanson · 2020-12-10T15:34:32Z

Would appreciate any input on this proposal @dhh -- thanks!

javan · 2020-12-10T15:55:15Z

Makes sense to support utility-first CSS conventions, but overloading the same data attribute to mean one or many class names seems problematic. If you can't trust this.fooClass to return a single class name then you can no longer do element.classList.add(this.fooClass) safely. Updating the singular property to return the plural property's first item might work. That's more or less what target properties do under the hood.

dhh · 2020-12-10T16:08:15Z

@javan You don't like having fooClass just return the string and then fooClasses return an array where split has been called on the string? Then they're just two types of accessors. One always returns a string, the other always returns an array (even if it might only have one element, if there are no spaces).

swanson · 2020-12-10T16:09:24Z

Makes sense to support utility-first CSS conventions, but overloading the same data attribute to mean one or many class names seems problematic. If you can't trust this.fooClass to return a single class name then you can no longer do element.classList.add(this.fooClass) safely. Updating the singular property to return the plural property's first item might work. That's more or less what target properties do under the hood.

Yeah, I wasn't sure if it was preferable to have the singular property blow up when using classList APIs if you pass a string with spaces (to notify you that you very likely have a typo/bug) or have it silently not apply anything other than the first CSS class.

Is throwing a Stimulus error (not the raw DomException) an option? Personally, I think I'd rather have that happen but I can see wanting to minimize possible errors in the library.

javan · 2020-12-10T16:20:17Z

You don't like having fooClass just return the string and then fooClasses return an array where split has been called on the string?

Using the example above:

<div data-controller="highlight" 
     data-highlight-pulse-class="animate-pulse ring-8 ring-teal-600 ring-offset-8 rounded">

I don't like this.pulseClass returning the full string "animate-pulse ring-8 ring-teal-600 ring-offset-8 rounded" because it's not a singular class name and will throw an exception if you classList.add(this.pulseClass). I think it should return the first class instead, "animate-pulse". Just like targets.

dhh · 2020-12-10T16:22:51Z

Yeah, that's a great point. I buy that.

javan · 2020-12-10T16:42:40Z

Is throwing a Stimulus error (not the raw DomException) an option? Personally, I think I'd rather have that happen but I can see wanting to minimize possible errors in the library.

Singular properties should throw an error. They do currently: https://github.com/stimulusjs/stimulus/blob/11370bc37ac07753dbc4d676eebb8534bb03d3f8/packages/%40stimulus/core/src/class_properties.ts#L18-L26

Unsure if plural properties should do the same.

…only

swanson · 2020-12-10T16:57:32Z

Singular properties should throw an error. They do currently:

Sorry, I was unclear. I meant should we throw a different error ("Expected a string without spaces, did you mean fooClasses?"). Sounds like returning the first class was the preferred option.

Unsure if plural properties should do the same.

Yeah, I'm not sure either. Plural targets do not check for missing the key (maybe that is intentional to support 0..N targets, not 1..N targets?).

But in the utility CSS use-case, it would be nice if Stimulus errored if you typo the key or omit it. You're unlikely to use the singular property if you are expecting multiple classes so you are skipping the missing attribute checks.

Trying to think if there is any use-case where you would intentionally want to pass in an empty string for the class value...

javan

Thanks for taking this on! Requesting a few minor changes.

docs/reference/css_classes.md

packages/@stimulus/core/src/tests/modules/class_tests.ts

packages/@stimulus/core/src/class_map.ts

swanson · 2020-12-14T19:59:30Z

@javan thank you for reviewing. I've made the requested changes.

swanson · 2020-12-28T02:02:19Z

Holler if there is anything else we want to do with this, I have some spare time during the holidays. Otherwise, I think it's ready for final review/merge. I can imagine the past few weeks have been pretty hectic so I appreciate the time to review this!

javan · 2020-12-28T13:40:13Z

Thanks, @swanson! We got a little sidelined getting hotwire out the door, and most of us are out for holidays now. Will resume soon!

swanson · 2021-02-01T17:59:11Z

👋 Friendly bump :)

sstephenson · 2021-02-01T19:06:21Z

This is at the top of our list when we return to Stimulus later this month 👍

Add [key]Classes method to better handle multiple CSS classes

sstephenson · 2021-03-24T22:29:52Z

Wonderful work on this all around, @swanson! Merged with a few revisions to the documentation: ce942f9

janko · 2021-05-20T12:00:01Z

For anyone wanting to use this functionality while waiting on the next Stimulus release, this is what I came up with:

// app/javascripts/controllers/application_controller.js
import { Controller } from 'stimulus'

export default class extends Controller {
  // TODO: Remove when https://github.com/hotwired/stimulus/pull/344 gets released.
  initialize() {
    this.constructor.classes.forEach(logicalClass => {
      Object.defineProperty(this, `${logicalClass}Classes`, {
        get() {
          return this.[`${logicalClass}Class`].split(' ')
        }
      })
    })
  }
}

Other controllers can subclass from this controller.

marcoroth · 2021-05-20T12:05:03Z

You can also swap the stimulus dependency in your package.json to the latest dev-build, like so:

"stimulus": "https://github.com/hotwired/dev-builds/archive/stimulus/848e0c7.tar.gz"

tleish · 2021-08-31T07:38:23Z

packages/@stimulus/core/src/string_helpers.ts

@@ -9,3 +9,7 @@ export function capitalize(value: string) {
 export function dasherize(value: string) {
  return value.replace(/([A-Z])/g, (_, char) => `-${char.toLowerCase()}`)
 }
+
+export function tokenize(value: string) {
+  return value.trim().split(/\s+/).filter(content => content.length)


A simpler (and slightly faster) method of doing this is

return value.match(/[^\s]+/g)

Please do add a PR 👍

Add [key]Classes method to better handle multiple classes

54e077e

swanson changed the title ~~Add [key]Classes method to better handle multiple classes~~ Add [key]Classes method to better handle multiple CSS classes Dec 8, 2020

Handle undefined edge case

f578181

swanson commented Dec 8, 2020

View reviewed changes

packages/@stimulus/core/src/class_properties.ts Outdated Show resolved Hide resolved

Accessing utility classes with singular property returns first class …

b96aa48

…only

Matt Swanson added 3 commits December 10, 2020 19:14

Style

1a4cd87

Add docs draft

f83aeed

Remove whitespace

6e19416

javan suggested changes Dec 14, 2020

View reviewed changes

Code review recycle

caecf0f

swanson commented Dec 14, 2020

View reviewed changes

packages/@stimulus/core/src/class_map.ts Outdated Show resolved Hide resolved

swanson requested a review from javan December 14, 2020 20:02

Refactor tokenize to string_helpers

aa01760

Base automatically changed from master to main January 12, 2021 20:57

sstephenson added a commit that referenced this pull request Mar 24, 2021

Merge pull request #344 from swanson/support-multiple-classes

59e0752

Add [key]Classes method to better handle multiple CSS classes

sstephenson merged commit aa01760 into hotwired:main Mar 24, 2021

marcoroth mentioned this pull request May 5, 2021

Proposal: Handling multiple classes like targets #399

Closed

tleish reviewed Aug 31, 2021

View reviewed changes

tleish mentioned this pull request Aug 31, 2021

A simpler (and slightly faster) method of tokenizing a string #430

Merged

swanson deleted the support-multiple-classes branch September 10, 2021 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add [key]Classes method to better handle multiple CSS classes #344

Add [key]Classes method to better handle multiple CSS classes #344

swanson commented Dec 8, 2020 •

edited

Loading

swanson commented Dec 10, 2020

javan commented Dec 10, 2020

dhh commented Dec 10, 2020

swanson commented Dec 10, 2020 •

edited

Loading

javan commented Dec 10, 2020

dhh commented Dec 10, 2020

javan commented Dec 10, 2020

swanson commented Dec 10, 2020

javan left a comment

swanson commented Dec 14, 2020

swanson commented Dec 28, 2020

javan commented Dec 28, 2020

swanson commented Feb 1, 2021 •

edited

Loading

sstephenson commented Feb 1, 2021

sstephenson commented Mar 24, 2021

janko commented May 20, 2021 •

edited

Loading

marcoroth commented May 20, 2021

tleish Aug 31, 2021 •

edited

Loading

dhh Aug 31, 2021

tleish Aug 31, 2021

Add [key]Classes method to better handle multiple CSS classes #344

Add [key]Classes method to better handle multiple CSS classes #344

Conversation

swanson commented Dec 8, 2020 • edited Loading

swanson commented Dec 10, 2020

javan commented Dec 10, 2020

dhh commented Dec 10, 2020

swanson commented Dec 10, 2020 • edited Loading

javan commented Dec 10, 2020

dhh commented Dec 10, 2020

javan commented Dec 10, 2020

swanson commented Dec 10, 2020

javan left a comment

Choose a reason for hiding this comment

swanson commented Dec 14, 2020

swanson commented Dec 28, 2020

javan commented Dec 28, 2020

swanson commented Feb 1, 2021 • edited Loading

sstephenson commented Feb 1, 2021

sstephenson commented Mar 24, 2021

janko commented May 20, 2021 • edited Loading

marcoroth commented May 20, 2021

tleish Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

dhh Aug 31, 2021

Choose a reason for hiding this comment

tleish Aug 31, 2021

Choose a reason for hiding this comment

swanson commented Dec 8, 2020 •

edited

Loading

swanson commented Dec 10, 2020 •

edited

Loading

swanson commented Feb 1, 2021 •

edited

Loading

janko commented May 20, 2021 •

edited

Loading

tleish Aug 31, 2021 •

edited

Loading