haltu / muuri

Infinite responsive, sortable, filterable and draggable layouts
https://muuri.dev
MIT License
10.78k stars 644 forks source link

support for multiple classes in 'containerClass' #445

Open giulianok opened 3 years ago

giulianok commented 3 years ago

We are currently using Tailwind to layout our website which provides a good amount of utility classes. This means that we usually use many classes in a single element in order to get what we expect.

One of the issues we are having is with containerClass because it only accepts one class.

The error is the following

Uncaught DOMException: Failed to execute 'add' on 'DOMTokenList': The token provided ('asd dada') contains HTML space characters, which are not valid in tokens.
    at <anonymous>:1:42

This is because the class is being applied as it's received (https://github.com/haltu/muuri/blob/20535eb9fe4f9abb345c572757aa427152ea4cd5/src/utils/addClass.js#L19).

classList.add function supports multiple classes but they have to be passed as N arguments (https://developer.mozilla.org/en-US/docs/Web/API/Element/classList).

A possible solution is

if (element.classList) {
  element.classList.add(...className.split(' '));
} else {
  ...
}
niklasramo commented 3 years ago

Yep, in general I've been thinking about supporting multiple class names too as it's not that big of a change. However, when using utility classes you can always just add/remove the class names manually to the element using element.classList() directly outside of Muuri. You can even set all of the Muuri's class name options to empty strings (and Muuri won't add any class names) and handle all the class name business fully outside Muuri in any manner you wish.

giulianok commented 3 years ago

true @niklasramo. The reason I think it will be a good idea is because:

  1. it will be cleaner
  2. devs are used to provide multiple classes from html and libs like React
  3. it will work just fine with libraries such as Muuri-react (UX might be compromised if we render and then we inject class names).
niklasramo commented 3 years ago

I'm pretty tempted to add this feature, but I still have one major issue in my mind. If you use utility classes and define e.g. "foo bar" as the item classnames and "bar baz" as the item positioning classnames Muuri would remove the "bar" classname from the item once positioning stops, which is probably not the wanted effect. Muuri currently assumes that the classnames provided via options are unique relative to each other and adding logic that would make the classnames work nicely even if there were overlapping names is not so simple anymore.

giulianok commented 3 years ago

I see your point. In case we use a single class for the container and another single class for an item, how does Muuri know that those classes should be kept? I believe it stores some reference, and I'm wondering how hard would be supporting multiple references. Feel free to point some directions, we're open to contributing if the change makes sense and it's useful for more people

As of right now, we just composed a custom class with Tailwind

niklasramo commented 3 years ago

Yes, Muuri keeps the class names in memory so it can add/remove them when needed. Supporting multiple references would not be hard, I guess, but there is still the problem which I mentioned above (about conflicting class names). Actually, the conflicting class names issue exist in Muuri already, e.g. if you would do this:

const grid = new Muuri(elem, {
  itemClass: 'item',
  itemPositioningClass: 'item',
});

Muuri would:

  1. Add item class to the item element on init.
  2. Add item class to the item element on positioning start (this would effectively do nothing as item element would already have the class).
  3. Remove item class from the item element on positioning end, which means that the item would not have item class anymore when the item is not positioning, which is probably not what user expects.

I'm not sure how to approach this problem though, should Muuri handle conflicting class names in some smart manner or just blindly obey the user provided config (as it does now)? In any case, I guess we could add support for providing multiple class names and leave it up to the user of the lib to make sure that there are no conflicting class names.