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

Can't target root with a dynamic tag name #147

Closed
jaydenseric opened this issue Mar 9, 2017 · 31 comments
Closed

Can't target root with a dynamic tag name #147

jaydenseric opened this issue Mar 9, 2017 · 31 comments

Comments

@jaydenseric
Copy link
Contributor

You can't target a root element with a dynamic tag name, using className:

export default ({children, level = 2}) => {
  const Heading = `h${level}`
  return (
    <Heading className='root'>
      {children}
      <style jsx>{`
        .root {
          color: red;
        }
      `}</style>
    </Heading>
  )
}
@giuseppeg
Copy link
Collaborator

@jaydenseric interesting use case :)
This is not possible right now because you can't target components at all.

Maybe we can make an exception for component names defined within the scope of the current component.

As for now you can wrap children in a span and style that one instead.

@jaydenseric
Copy link
Contributor Author

jaydenseric commented Mar 29, 2017

Another use case I encounter is to be able to set a TextInput component to either a textarea or input element via a multiline prop. It's not an option to add an inner wrapper for styling; the workaround is to define two components and return the right one with a conditional.

@jabacchetta
Copy link
Contributor

jabacchetta commented Oct 7, 2017

I believe I'm having a similar issue. I'm attempting to use dynamic tag names in combination with className.

import React from 'react'

import styles from './styles'

type Props = {
  children: string,
  Element: 'span' | 'p' | 'h1' | 'h2' | 'h3',
  kind: 'smTitle' | 'xlTitle',
}

// class declaration is lost
const TextBroken = ({ children, Element, kind }: Props) => (
  <Element className={kind}>
    {children}
    <style jsx>{styles}</style>
  </Element>
)

// works, but requires a span wrapper
const TextWorking = ({ children, Element, kind }: Props) => (
  <Element>
    <span className={kind}>{children}</span>
    <style jsx>{styles}</style>
  </Element>
)

@simonsmith
Copy link

simonsmith commented Oct 16, 2017

Bumped into this issue almost immediately when trying to add a className to a <Link> component from react-router :)

I've tried the suggested workaround but in this case <Link/> renders an a element and I need to apply styling directly to that element.

Any thoughts on merging #244?

@giuseppeg
Copy link
Collaborator

giuseppeg commented Oct 17, 2017

@simonsmith the problem with passing the parent scoped className to components is that it may lead to unexpected side effects like descendants being styled due to generic selectors defined in the parent. Eg.

// descendant
// className meant to be any className i.e. not added on purpose to pass styled-jsx scoped styles
const Child = ({className, children}) => (
   <div className={'hello ' + className}>{children}</div>
)
// parent

const Parent = ({children}) => (
   <div>
         {children}
         <Child>hello</Child> {/* didn't plan on passing any className */}
        <style jsx>{`div { margin: 50px  }`}</style> {/* since styled-jsx passes the className to Child the inner one will get the margin too */}
   </div>
)

To solve this issue maybe we could allow components selectors like

<style jsx>{`Child { color: red  }`}</style>

which generates a unique className for that component, not sure if this would work either.

Also not every 3rd party component is made to accept a className prop. It would be cool to find a generic pattern to do both, ideas are welcome!

@simonsmith
Copy link

simonsmith commented Oct 17, 2017

@giuseppeg If the className is generated in the parent and passed to the Child how does that interfere? Doesn't it end up being like this:

.jsx-123456.some-class { /*defined in parent component */ }

<div class="parent">
  <div class="child jsx-123456 some-class">
</div>

You may have to school me on how styled-jsx is operating behind the scenes.

Regardless, any thoughts on how I work around the issue with the <Link> component?

@giuseppeg
Copy link
Collaborator

@simonsmith jsx-123456 is also applied to the outer div (.parent).

To style the Link you could temporarily use :global() until we find a better mousetrap:

<div>
   <Link className="my-link">something</Link>
   <style jsx>{`div > :global(.my-link) { color: red }`}</style>
</div>

@arsen
Copy link

arsen commented Dec 6, 2017

styled-jsx is pretty amazing, simple and powerful, except the lack of this super obvious functionality, that anyone would expect from css-in-js library, which is a show stopper.

Do you guys have any plans to support styling custom-defined user components without using :global ?

@giuseppeg
Copy link
Collaborator

@arsen that's on my radar, probably the next most important feature to add!

@iamawebgeek
Copy link

iamawebgeek commented Jan 16, 2018

@giuseppeg What should I do if I can't wrap my component with a div/span
I have following code:

class Button extends React.Component {
  render() {
    return (
      <React.Fragment>
        <style jsx={true}>{`.button { color: white; }`}</style>
        <button className={`button ${this.props.className}`}>
          {this.props.children}
        </button>
      </React.Fragment>
    )
  }
}

class CoolButton extends React.Component {
  render() {
    return (
      <React.Fragment>
        <style jsx={true}>{`.cool-button { border: 1px solid #808080; }`}</style>
        <Button className={`cool-button ${this.props.className}`}>
          {this.props.children}
        </Button>
      </React.Fragment>
    )
  }
}

I just want to extend my component this way, but it is not working, please, suggest some other solution.
It would be awesome if there was a pseudo selector like :root in Angular 4, after which we could use :global(.our-class) and be able to extend all components and elements inside that same component without adding any root tag and extend it.

@a-ignatov-parc
Copy link
Contributor

@iamawebgeek this should work

class Button extends React.Component {
  render() {
    return (
      <button className={`button ${this.props.className}`}>
        <style jsx>{`.button { color: white; }`}</style>
        {this.props.children}
      </button>
    )
  }
}

class CoolButton extends React.Component {
  render() {
    const scoped = resolveScopedStyles(
      <scope>
        <style jsx>{`.cool-button { border: 1px solid #808080; }`}</style>
      </scope>
    );

    const classNames = [
      scoped.className,
      'cool-button',
      this.props.className,
    ];

    return (
      <React.Fragment>
        <Button className={classNames.join(' ')}>
          {this.props.children}
        </Button>
        {scoped.styles}
      </React.Fragment>
    )
  }
}

@iamawebgeek
Copy link

@a-ignatov-parc Awesome, it worked, thanks. Is there any way to make it more simple like just wrapping everything in scope tag?

@a-ignatov-parc
Copy link
Contributor

Well, you can modify initial resolveScopedStyles helper to something like this

function resolveScopedStyles(scope) {
  return ({ children }) => {
    children.props.className += ' ' + scope.props.className;

    return (
      <React.Fragment>
        {children}
        {scope.props.children}
      </React.Fragment>
    )
  };
}

And then use it as className prefixer for single react element

class CoolButton extends React.Component {
  render() {
    const Scoped = resolveScopedStyles(
      <scope>
        <style jsx>{`.cool-button { border: 1px solid #808080; }`}</style>
      </scope>
    );

    return (
      <Scoped>
        <Button className={`cool-button ${this.props.className}`}>
          {this.props.children}
        </Button>
      </Scoped>
    );
  }
}

@giuseppeg
Copy link
Collaborator

When the styles are static it is better to define then outside of the render method otherwise they are treated as dynamic.

@a-ignatov-parc
Copy link
Contributor

otherwise they are treated as dynamic.

@giuseppeg I thought that

const styles = (
  <style jsx>{`
    .foo { ... }
  `}</style>
);

function Component() {
  return (
    <div>
      {styles}
    </div>
  )
}

Is same as

function Component() {
  return (
    <div>
      <style jsx>{`
        .foo { ... }
      `}</style>
    </div>
  )
}

Am I wrong? 🤔

Or you talking about?

const styles = css`
   .foo { ... }
`;

function Component() {
  return (
    <div>
      <style jsx>{styles}</style>
    </div>
  )
}

@giuseppeg
Copy link
Collaborator

@a-ignatov-parc sorry I made some confusion because I didn't look properly at the examples. Styles that reference variables defined within the scope of the render method are treated as dynamic. Eg.

render() {
  const color = 'red'
  return <div>
        {this.props.children} 
         <style jsx>{`div { border: 1px solid ${color}; }`}</style>
    </div>
}

If color is static just define it outside of the component.

The following syntax is not supported I think:

const styles = (
  <style jsx>{`
    .foo { ... }
  `}</style>
);

function Component() {
  return (
    <div>
      {styles}
    </div>
  )
}

Wheras the one below (css) produces both global and scoped styles:

const styles = css`
   .foo { ... }
`;

function Component() {
  return (
    <div>
      <style jsx>{styles}</style>
    </div>
  )
}

Somebody needs to work on improving this last behavior #345

@a-ignatov-parc
Copy link
Contributor

@giuseppeg thanks for clarification 👍

@arsen
Copy link

arsen commented Mar 22, 2018

Any progress on this? :)

@giuseppeg
Copy link
Collaborator

giuseppeg commented Apr 8, 2018

How would you folks feel about adding support for a special styled-jsx props that will add the scoped classname to components?

<Heading className="root" styled-jsx>
      {children}
      <style jsx>{`
        .root {
          color: red;
        }
      `}</style>
</Heading>

@a-ignatov-parc
Copy link
Contributor

Maybe for base scenarios, we should prefix className (if exists) without any additional props? 🤔

I think most of the developers are expecting such behavior.

@giuseppeg
Copy link
Collaborator

so you mean automatic detection (whitelisting) for the use case of the OP?

@a-ignatov-parc
Copy link
Contributor

I thought that we can prefix className prop for all react elements (DOM + Components) if it's specified as a string (and maybe some other cases). For advanced scenarios, we can use css.resolve.

Also, this behavior can be changed in babel's plugin options. By default, I'd suggest to leave it disabled.

What do you think @giuseppeg?

@giuseppeg
Copy link
Collaborator

can you show me an example?

@a-ignatov-parc
Copy link
Contributor

<Heading>
  <style jsx>{`
    .root {
      color: red;
    }
  `}</style>
</Heading>

Transforms to

<Heading>
  <JSXStyle id="xxx" />
</Heading>

<Heading className="root">
  <style jsx>{`
    .root {
      color: red;
    }
  `}</style>
</Heading>

Transforms to

<Heading className="xxx root">
  <JSXStyle id="xxx" />
</Heading>

But

function MyComp({ classes }) {
  return (
    <Heading className={classes}>
      <style jsx>{`
        .root {
          color: red;
        }
      `}</style>
    </Heading>
  );
}

Should transform to

function MyComp({ classes }) {
  return (
    <Heading className={classes}>
      <JSXStyle id="xxx" />
    </Heading>
  );
}

@a-ignatov-parc
Copy link
Contributor

I think if className prop is static and is a string we can prefix it.

@giuseppeg
Copy link
Collaborator

I think that it makes sense to automatically scope (add the className) in the case reported by the OP. In any other case we should scope only when the user adds a prop like styled-jsx otherwise people can use css.resolve i.e. prefer explicitness over magic

@giuseppeg giuseppeg reopened this Apr 9, 2018
@twltwl
Copy link
Contributor

twltwl commented Jul 9, 2018

Any progress on this ? 😺

@giuseppeg
Copy link
Collaborator

@twltwl hi, want to pick this up? I can offer guidance

@twltwl
Copy link
Contributor

twltwl commented Jul 10, 2018

@giuseppeg Yes I could. Some pointers on where to start would be apprichiated :)

@giuseppeg
Copy link
Collaborator

@twltwl sure, first of all you should branch off next (no master, we are going to include this fix in styled-jsx v3). Then you should try to add this code:

const tag = path.get('name')

if (
  name &&
  name !== 'style' &&
  name !== STYLE_COMPONENT &&
  (
    name.charAt(0) !== name.charAt(0).toUpperCase() ||

    /*
      Allows tags references:
      const Heading = `h{props.level}`

      return <Heading>hi</Heading>
    */
    Object.values(path.scope.bindings).some(binding => 
      binding.referencePaths.some(r => r === tag)
    )
  )
) {

Here https://github.com/zeit/styled-jsx/blob/a55c821a998b9a7210c03a30c4ae33de7ec94eb6/src/babel.js#L43-L48

And then add some tests here: https://github.com/zeit/styled-jsx/blob/next/test/fixtures/attribute-generation-classname-rewriting.js

Add some new components and then run npm test from the root of the project. Make sure you test edge cases, eg. Header is defined outside of the component.

@giuseppeg giuseppeg added this to the v3 milestone Jul 18, 2018
@giuseppeg
Copy link
Collaborator

fixed in #462 will ship with styled-jsx v3

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

Successfully merging a pull request may close this issue.

8 participants