Why do all object keys for className overlayClassName object prop need to be required?

[indiesquidge]

Summary:

#536 introduced a specification for the className and overlayClassName props when passed as objects. I am interested in knowing whether or not all keys on the prop object need to be required. I would prefer it not be required to implement all three keys all the time (unless there is some a11y gain or the removal of some subtle bug that I'm missing).

Steps to reproduce:

1. Create a <ReactModal> and supply it an overlayClassName prop as an object, but exclude one of the three possible object keys (i.e. base, afterOpen, beforeClose)

2. Upon the modal render, you should see a prop warning in your console

Expected behavior:

No proptypes warning for missing object keys.

Link to example of issue:

I made a codepen that shows the warning, but I'm not sure it's necessary to show the point. It's obvious by the code that the proptype warning will occur.

[diasbruno]

Yeah, there is room to improve this particular function ModalPortal.js#L268. The way it's implemented requires the same shape as in line ModalPortal.js#L272.

[diasbruno]

Looking at the code now, currently it works like this:

<Modal className={{ 
    base: "mymodal",
    afterOpen: "mymodal_after-open",
    beforeClose: "mymodal_before-close"
}}>

This will render to className="mymodal mymodal_after-open".

Not sure if afterOpen and beforeClose should be only the suffix part of the class, like:

<Modal className={{ 
    base: "mymodal",
    afterOpen: "_after-open",
    beforeClose: "_before-close"
}}>

This will give the same result as the first example, ${base}${afterOpen} and ${base}+${beforeClose}.

[diasbruno]

@indiesquidge any thoughts?

[indiesquidge]

Hey, @diasbruno. I have been away on holiday vacation the last few weeks. Let me familiarize myself with this this week and get back to you soon! :)

[indiesquidge]

@diasbruno, after digging back into the necessity of having every key be required on the className and overlayClassName props as objects, it seems like there are a number of ways in which we can allow the keys to be optional. Looking back through this library's history, it seems the reason the keys were made required was because a missing key resulted in a class name of undefined being added to the element. If that is indeed the only reason they were added, it seems like we could get around that in a number of ways.

Potential Solution 1

Check if the key exists, and if it doesn't exist, only include base. (We could either leave base as a required key, or just default it to an empty string within this function.)

  buildClassName = (which, additional) => {
    const classNames =
      typeof additional === "object"
        ? additional
        : {
            base: CLASS_NAMES[which],
            afterOpen: `${CLASS_NAMES[which]}--after-open`,
            beforeClose: `${CLASS_NAMES[which]}--before-close`
          };
    let className = classNames.base;
    if (this.state.afterOpen) {
-     className = `${className} ${classNames.afterOpen}`;
+     className = classNames.afterOpen
+       ? `${className} ${classNames.afterOpen}`
+       : className;
    }
    if (this.state.beforeClose) {
-     className = `${className} ${classNames.beforeClose}`;
+     className = classNames.beforeClose
+       ? `${className} ${classNames.beforeClose}`
+       : className;
    }
    return typeof additional === "string" && additional
      ? `${className} ${additional}`
      : className;
  };

Potential Solution 2

Just render an empty string if the key is missing. This is less code, but the downside is that the full class name for the opened/closed states becomes customBaseClass with that extra space there on the end. I don't see that as a huge issue, but it could be perceived as strange.

  buildClassName = (which, additional) => {
    const classNames =
      typeof additional === "object"
        ? additional
        : {
            base: CLASS_NAMES[which],
            afterOpen: `${CLASS_NAMES[which]}--after-open`,
            beforeClose: `${CLASS_NAMES[which]}--before-close`
          };
    let className = classNames.base;
    if (this.state.afterOpen) {
-     className = `${className} ${classNames.afterOpen}`;
+     className = `${className} ${classNames.afterOpen || ""}`;
    }
    if (this.state.beforeClose) {
-     className = `${className} ${classNames.beforeClose}`;
+     className = `${className} ${classNames.beforeClose || ""}`;
    }
    return typeof additional === "string" && additional
      ? `${className} ${additional}`
      : className;
  };

There are probably many ways to get around adding an undefined class name. If that is the reason the keys were made required, I'd be more than happy to implement an internal solution around it with some tests. If there are other reasons for making the keys required, I'd love to know about them so that I can contemplate a solution with more context.

[diasbruno]

You are right about the 'missing the default'. This problem could be fixed by merging with some default object like:

const defaultNames = {
  base: CLASS_NAMES[which],
  afterOpen: `${CLASS_NAMES[which]}--after-open`,
  beforeClose: `${CLASS_NAMES[which]}--before-close`
};
typeof additional === "object"
  ? Object.assign({}, defaultNames, additional) <--
  : default;

Maybe this function need to be refactored since it can do unnecessary work depending on the type of additional.

[kaushik94]

@diasbruno also I think the npm package https://www.npmjs.com/package/classnames can be an alternative to this.