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

Test cases not completely fair #41

Open
jfrej opened this issue Apr 19, 2018 · 2 comments
Open

Test cases not completely fair #41

jfrej opened this issue Apr 19, 2018 · 2 comments

Comments

@jfrej
Copy link

jfrej commented Apr 19, 2018

The emotion css-mode test is not actually using emotion for colouring of the cells, which is the heaviest part. It just uses inline styles:
style={{ background: `rgba(74, 174, 53, ${x})` }}
which has to be super quick.

Instead, it should be using class generation like the glamorous/glamor-css test does.

I can submit a PR to fix this. It considerably changes the results table.

@jfrej
Copy link
Author

jfrej commented Apr 20, 2018

Ok, I see how I got it a bit wrong. The results table mentions inline styles, so perhaps the test case is not wrong but I consider it misleading. I don't really see the point of comparing a fully dynamic class creation (like in the emotion-simple case) with a solution that uses inline styles.
At best there's a case missing for a fully dynamic solution using emotion's css() function.
Same with aphrodite and cxs.
I'm preparing a PR to add them.

@jfrej jfrej changed the title Emotion css-mode test case is wrong Test cases not completely fair Apr 20, 2018
@grebenyuksv-preply
Copy link

grebenyuksv-preply commented Dec 11, 2019

I think there's a naming problem. I also got misled by the results stating that styled-jsx-inline-styles is blazingly fast, whereas there's just 1 static <style jsx> tag inside, everything else done with plain inline styles.

What I had to check myself, however, was the case of many static <style jsx> tags, which shows that there's only a small (270 vs 320) difference between N static tags and N dynamic ones. Here's the case I coded myself:

const Cell = ({ value, children }) => (
  <div className="cell">
    {children}
    <style jsx>{`
      .cell {
        display: table-cell;
        padding: 10px;
      }
    `}</style>
    <style jsx>{`
      .cell {
        background: rgba(74, 174, 53, 30);  // here I replaced the var with a static value
      }
    `}</style>
  </div>
);

Thanks for this great repo anyway, it saved me a LOT of time + your bootstrap was so easy to extend with what I wanted.

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

No branches or pull requests

2 participants