krasimir / react-cssx

CSS in React
MIT License
66 stars 7 forks source link

Is the wrapper necessary? #5

Open zvictor opened 8 years ago

zvictor commented 8 years ago

I wonder if the <div /> element created by the <CSSX /> wrapper is actually needed. Right now we are forcing an interference in the tree of elements descendants of the wrapper. It could create problems and break some things, making it harder to migrate from a different solution to react-cssx.

I understand that React is not very friendly when we try to create "transparent wrappers", but I have seen solutions that managed to do it well, not leaving any trace of their existence in the DOM. E.g. react-container-dimensions.

I also understand that right now there is a need to have a wrapper in order to prepend all the css rules with an id selector (e.g. #cssx-el-2 p). However, I believe that it could be replaced by attribute selectors, setting a given attribute (e.g. <p data-cssx="el-2" />) to all the top children inside the wrapper.

Any thoughts on that? I would like to hear the community and the author's opinion before I consider a PR to address this.

krasimir commented 8 years ago

When I read your comment I first agree that using children API is actually cleaner. I mean not having a wrapper. But then realized that we have to modify the top node so we get the css applied (the idea with the attribute selector). I'm not sure that modifying an element which is not ours is a good idea. I prefer keeping the wrapper and don't touch the other markup. What's missing now is to customize the <div>. It's not good that react-cssx uses a block element always. The developer should decide what type of node wrapper to be.

zvictor commented 8 years ago

The customisable element can be achieved by defining an attribute in similar way to how flexbox-react does it with the element attribute.

I have to say, however, that I don't sympathize with this solution. First time I saw a <Flexbox element="header" ... in our project's codebase I though that header was only used internally by the component's logic for something else other than rendering a <header>. It is just not intuitive enough.


The attribute selector idea is valid HTML markup and, implemented in the right way, doesn't present any risks. Radium for instance uses it intensively (data-radium="true") and apparently the community has accepted it well.

zvictor commented 8 years ago

@krasimir this issue is blocking the development of parts of my product and I need to decide on which way to go from here. Have you considered what you want to have done to solve the question?

krasimir commented 8 years ago

@zvictor can you please try the new 1.2.0 version. There is data-element attribute supported. It's added to the README here.

zvictor commented 8 years ago

@krasimir I appreciate your effort in solving this issue, however it has never been about the element used by the wrapper. Let me give you some real world cases to clarify the question.

Let's say that I have a website using semantic-ui items and I want to customize their look using CSSX. The code would be something like this, considering I want to separate the style of .item from .items:

const List({ items }) => (
  <CSSX styles={listStyle()}>
    <div className="ui items">
      {{ items }}
    </div>
  </CSSX>
);

const Item({ children }) => (
  <CSSX styles={itemStyle()}>
    <div className="item">
      {{ children }}
    </div>
  </CSSX>
);

resulting in something like this:

<div id="..."> // CSSX
  <div className="ui items"> // Semantic
    <div id="..."> // CSSX
      <div className="item"> // Semantic
        ...
      </div>
    </div>
  </div>
</div>

which breaks the semantic-ui design, based in the rule .ui.items > .item.

Besides breaking external css structures, another big problem with the current implementation is that I cannot style the wrappers themselves. I end up having a lot of wrapper elements that happen to be totally uncontrollable design-wise, serving the only purpose of being anchors.