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 full functionality of classnames package to classList #88

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kcirtaptrick
Copy link

@kcirtaptrick kcirtaptrick commented Sep 13, 2021

This allows classList to accept an array of classNames, omitting falsey values (except for 0, reasoning described in comment).

This will allow users to put optional classNames in className, and not have to worry about omitting an unset value. The following shows the common usecase of passing through a classname:

// BEFORE
function Component({ className }) {
  return <div className={`component-class-name ${className || ""}`}> // nit: this will have an extra space and removing it is much more verbose
    ...
  </div>;
}

// AFTER
function Component({ className }) {
  return <div classList={['component-class-name', className]}>
    ...
  </div>
}

This is fully backwards compatible and accepts objects & nested arrays:

function Collapsible({ className, classList }) {
  const [collapse, setCollapse] = createSignal(false);

  return <div classList={['collapsible', { collapse: collapse() }, className, classList]}>
    ...
  </div>;
}

@ryansolid
Copy link
Owner

I've been slow on this one (and it has come up before) mostly in that we need to apply the support in at least 4 places including client and server side compiler and runtimes. For the most part I skip using these helpers if the compiler can analyze what we need(ie.. if it is a literal in the binding we know that it's fixed if there are no spreads or no computed properties).

But unclear if this array form can apply the same compiler level optimization. Probably if we inline a conditional. Like right now I don't optimize {[className]: true} which is pretty much identical to this. It's a bit unfortunate if arrays are just de-opt in general but maybe we can do something about this.

One thing I wasn't clear about on this part of the solution was does it handle unsetting properly and with arrays do we have to consider order? Like if someone passed in [aString(), anObject()] or even a anArray() that could apply class rules differently like if the array was made new each time with arguments in different orders.

@kcirtaptrick
Copy link
Author

But unclear if this array form can apply the same compiler level optimization.

Hmm I'm not familiar with how the compiler handles classList, can you please point me in the right direction?

One thing I wasn't clear about on this part of the solution was does it handle unsetting properly and with arrays do we have to consider order?

Regarding order, I'm not sure if I understand the issue, but this current solution uses existing functionality that clears and rewrites the dom classList which should work regardless of order.

Comment on lines 89 to 94
for (i = 0, len = prevKeys.length; i < len; i++) {
const key = prevKeys[i];
if (!key || key === "undefined" || key in value) continue;
toggleClassKey(node, key, false);
delete prev[key];
}
Copy link
Owner

Choose a reason for hiding this comment

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

This loop compares prev keys that and if they are missing from the value it unsets them. I'm gathering we need to update to support objects in arrays.

Copy link
Author

@kcirtaptrick kcirtaptrick Sep 15, 2021

Choose a reason for hiding this comment

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

Oh mb, missed the key in value, I have a couple of solutions in mind. First I'm thinking the simplest way to achieve this is to just wipe them all and rebuild them. At first that sounds like it would have poor performance, but I think its unlikely developers will be setting enough classnames to make this significant, even on several components. Another option would be to track present classes with setClasses, and remove all other classes.
I'll draft up these 2 scenarios so you'll have some more context.

Comment on lines 89 to 94
for (i = 0, len = prevKeys.length; i < len; i++) {
const key = prevKeys[i];
if (!key || key === "undefined" || key in value) continue;
toggleClassKey(node, key, false);
delete prev[key];
}
Copy link
Author

Choose a reason for hiding this comment

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

Scenario 1, wipe all classes. These will be rebuilt in setClasses

Suggested change
for (i = 0, len = prevKeys.length; i < len; i++) {
const key = prevKeys[i];
if (!key || key === "undefined" || key in value) continue;
toggleClassKey(node, key, false);
delete prev[key];
}
for (const key of Object.keys(prev)) {
toggleClassKey(node, key, false);
delete prev[key];
}

Comment on lines +95 to +116
(function setClasses(classes) {
if (classes instanceof Array) {
for (const key of classes) {
// classList setting classes for all numbers except for 0 may be
// unexpected behavior, you must explicity omit 0 using `n || false`
if (!key && key !== 0) continue;
if (typeof key === 'object') { // Both objects and arrays
setClasses(key);
continue;
}
toggleClassKey(node, key, true);
prev[key] = true;
}
} else if (typeof classes === 'object') {
for (const [key, enabled] of Object.entries(classes)) {
if (!enabled) continue;
toggleClassKey(node, key, true);
prev[key] = true
}
}
})(value);

Copy link
Author

@kcirtaptrick kcirtaptrick Sep 15, 2021

Choose a reason for hiding this comment

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

Scenario 2, track classes & remove absent

Suggested change
(function setClasses(classes) {
if (classes instanceof Array) {
for (const key of classes) {
// classList setting classes for all numbers except for 0 may be
// unexpected behavior, you must explicity omit 0 using `n || false`
if (!key && key !== 0) continue;
if (typeof key === 'object') { // Both objects and arrays
setClasses(key);
continue;
}
toggleClassKey(node, key, true);
prev[key] = true;
}
} else if (typeof classes === 'object') {
for (const [key, enabled] of Object.entries(classes)) {
if (!enabled) continue;
toggleClassKey(node, key, true);
prev[key] = true
}
}
})(value);
const present = new Set();
(function setClasses(classes) {
if (classes instanceof Array) {
for (const key of classes) {
// classList setting classes for all numbers except for 0 may be
// unexpected behavior, you must explicity omit 0 using `n || false`
if (!key && key !== 0) continue;
if (typeof key === 'object') { // Both objects and arrays
setClasses(key);
continue;
}
toggleClassKey(node, key, true);
prev[key] = true;
present.add(key);
}
} else if (typeof classes === 'object') {
for (const [key, enabled] of Object.entries(classes)) {
if (!enabled) continue;
toggleClassKey(node, key, true);
prev[key] = true
present.add(key);
}
}
})(value);
for(const key of Object.keys(prev)) {
if(!present.has(key)) {
toggleClassKey(node, key, false);
delete prev[key];
}
}

also delete this snippet

  for (i = 0, len = classKeys.length; i < len; i++) {
    const key = classKeys[i],
      classValue = !!value[key];
    if (!key || key === "undefined" || prev[key] === classValue) continue;
    toggleClassKey(node, key, classValue);
    prev[key] = classValue;
  }

@kcirtaptrick
Copy link
Author

Now that I write these 2 scenarios down, the first one seems to me like the way to go, I don't imagine the second will have any significant performance benefit, but it does add plenty of complexity

@ANFADEV
Copy link

ANFADEV commented Jun 27, 2022

Hi, are there any updates on this?

@iongion
Copy link

iongion commented Jun 1, 2023

Recently discovered solid and I am toying around with porting an UI framework https://stackblitz.com/edit/solidjs-templates-gfcotu

Solid has classList - which is cool, but these are my findings

  • Although name says list, it is actually a dictionary
  • I can't ensure order, I don't know in what order the classes will be added to the element based on the keys of the object, this is a no go, especially when integrating with 3rd parties where I cannot control order of css
  • Limited expressivity

classnames npm package is 100 % perfect for this

      class={classNames(
        Classes.COLLAPSE,
        {
          [Classes.COLLAPSE_OPEN]: props.isOpen,
          [Classes.COLLAPSE_CLOSE]: !props.isOpen,
        },
        props.class
      )}

if props.class is undefined, how will that look like with classList version ?

    classList={{
      [Classes.COLLAPSE]: true,
      [Classes.COLLAPSE_OPEN]: props.isOpen,
      [Classes.COLLAPSE_CLOSE]: !props.isOpen,
      [props.class || ""]: true
    }}

Would really appreciate this PR being given considerations, other discussions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants