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

Switch from data attributes to className #247

Closed
giuseppeg opened this issue Jun 26, 2017 · 4 comments
Closed

Switch from data attributes to className #247

giuseppeg opened this issue Jun 26, 2017 · 4 comments

Comments

@giuseppeg
Copy link
Collaborator

giuseppeg commented Jun 26, 2017

To improve performances we should switch to use className instead of the data-jsx attribute.

When we have spread props eg. {...props} we would need to move the className attribute to the end and rewrite it so it applies classes on cascade.

<div {...props} className="foo" {...rest} />
// compiles to
<div {...props} {...rest} className={`${rest.className != null && rest.className || 'foo'} jsx123`} />

I actually started to work on it (didn't go far though so contribution are still welcome!) and here is the test case:

export default () => (
  <div className='test'>
    <div className={test && test()} />
    <div className={test && 'test'} />
    <div className={`test ${test ? 'test' : ''}`} />
    <div className={a + 'test'} />
    <div className={`test ${test ? 'test' : ''}`} {...props} />
    <div className='hi' {...props} {...rest} />
    <div {...props} />
    <div {...props} {...rest} />
    <div />
    <style jsx>{'div { color: red }'}</style>
  </div>
)

Which would compile to (just showing some examples not all of them):

// without className and spreads
<div className={`jsx123`} />

// with existing className
<div className={`test jsx123`} />

// with existing className and a spread
<div {...props} className={`${props.className != null && props.className || 'test'} jsx123`} />

// with existing className and many spread
<div {...props} {...rest} className={`${props.className  != null && props.className || rest.className != null && rest.className || `test ${test ? 'test' : ''}`} jsx123`} />

// with a spread only
<div {...props} className={`${props.className != null && props.className || ''} jsx123`} />

// with many spread only
<div {...props} {...rest} className={`${props.className != null && props.className || rest.className != null && rest.className || ''} jsx123`}  />

where jsx123 is the style-jsx unique classname.

@nkzawa
Copy link
Contributor

nkzawa commented Jul 11, 2017

SGTM

@pruhstal
Copy link

@giuseppeg this still in progress?

@giuseppeg
Copy link
Collaborator Author

@pruhstal I am working on #81 right now, this will probably be the next improvements

@giuseppeg giuseppeg mentioned this issue Sep 27, 2017
2 tasks
@giuseppeg
Copy link
Collaborator Author

Fixed by #288

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

No branches or pull requests

3 participants