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

Re-inserted conditional rules shouldn’t cause style duplication #43

Open
kripod opened this issue Jul 23, 2020 · 2 comments
Open

Re-inserted conditional rules shouldn’t cause style duplication #43

kripod opened this issue Jul 23, 2020 · 2 comments
Labels
enhancement New feature or request

Comments

@kripod
Copy link
Owner

kripod commented Jul 23, 2020

Motivation

When re-inserting a style for precedence management, the old rule isn’t removed from the stylesheet. This may cause some duplication when media or feature queries are used frequently.

@eddyw
Copy link

eddyw commented Jul 26, 2020

👋 When a conditional rule is re-inserted, it shouldn't be removed or duplicated but it should be generated with a different classname. This also applies when they aren't re-inserted in this case:

Take for example this:

export default function IndexPage(): JSX.Element {
  return (
    <React.StrictMode>
      <h1>Hello, world!</h1>
      <p
        className={css({
          "@media": {
            "(min-width: 5px)": { color: "blue" },
            "(min-width: 4px)": { color: "green" },
          },
        })}
      >
        Conditional rule precedence management test #1, should be green
      </p>
    </React.StrictMode>
  );
}

It is green and generates the following CSS code:

@media(min-width:5px){._bxnxkd{color:blue}}
@media(min-width:4px){._107ye90{color:green}}

which is the correct order in which they were defined.
Now try this:

export default function IndexPage(): JSX.Element {
  return (
    <React.StrictMode>
      <h1>Hello, world!</h1>
      <p
        className={css({
          "@media": {
            "(min-width: 4px)": { color: "green" },
            "(min-width: 5px)": { color: "blue" },
          },
        })}
      >
        Conditional rule precedence management test #2, should be blue
      </p>
    </React.StrictMode>
  );
}

It is blue and generates again in the CSS in correct order:

@media(min-width:4px){._107ye90{color:green}}
@media(min-width:5px){._bxnxkd{color:blue}}

However, because re-inserted conditional rules have the same class-name, this happens:

export default function IndexPage(): JSX.Element {
  return (
    <React.StrictMode>
      <h1>Hello, world!</h1>
      <p
        className={css({
          "@media": {
            "(min-width: 5px)": { color: "blue" },
            "(min-width: 4px)": { color: "green" },
          },
        })}
      >
        Conditional rule precedence management test #1, should be green
      </p>
      <p
        className={css({
          "@media": {
            "(min-width: 4px)": { color: "green" },
            "(min-width: 5px)": { color: "blue" },
          },
        })}
      >
        Conditional rule precedence management test #2, should be blue
      </p>
    </React.StrictMode>
  );
}

Both are green and CSS is:

@media(min-width:5px){._bxnxkd{color:blue}}
@media(min-width:4px){._107ye90{color:green}}

The expected result should be:

@media(min-width:5px){._bxnxkd{color:blue}} /* SourceIndex: 0 */
@media(min-width:4px){._107ye90{color:green}} /* SourceIndex: 1 */
@media(min-width:4px){._NEW_CLS_0{color:green}} /* SourceIndex: 0 */
@media(min-width:5px){._NEW_CLS_1{color:blue}} /* SourceIndex: 1 */

I couldn't understand very well what createInstance does (the terms used could use some comments 😅 ) but ..
The hash method could also use precedence and ruleIndex to generate the classname (sort of like hash(declarations + precedence + ruleIndex || 0)), so you could always check if a rule has been inserted instead of re-inserting the rule and removing the old one which would break styling (as shown above)

@kripod
Copy link
Owner Author

kripod commented Jul 26, 2020

Awesome, I really love this idea! It would allow conditional rules to be defined in any order without issues. (A mobile-first approach is still recommended and should possibly generate a shorter output, though.)

Your examples are well-detailed and will help me a lot when working on the implementation, possibly next week. Thank you so much! 🙌

As for the documentation, an overhaul would be nice to have, especially for developers like you, who dive deep into the details. The createInstance method works similarly to the concept of a class, as it returns the css and keyframes methods to be used throughout an application. A default instance is created and set up for a better developer experience. I'm not sure about the value of custom instances, though, as their only purpose is allowing injection of styles into <iframe> elements.

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

No branches or pull requests

2 participants