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

Dynamic style composition with cx and the css prop breaking & + & selectors #1417

Closed
DesignerDave opened this issue Jun 25, 2019 · 9 comments

Comments

@DesignerDave
Copy link

Current behavior:

With either the emotion or @emotion/core packages, using the & + & selector as part of a call to css() that is composed with dynamically-set styles causes the selector to not work as intended.

To reproduce:

Reproduced with @emotion/core using the CSS prop: https://codesandbox.io/s/kind-jepsen-fve70
Reproduced with the emotion package using cx: https://codesandbox.io/s/proud-water-6uzkl

  1. Write a set of CSS that is "static" (no string interpolation), that includes the & + & selector.
  2. Write a component that has styles set based on a passed-in prop (using string interpolation), and compose those with the styles written as a part of step 1 (via cx or the css prop).
  3. Render multiple of these components as siblings.

If you follow the above steps, sibling elements will not be selected as you would expect with the & + & selector. The <style> tags added to the page appear to include a & + & selector with different classNames for each instance of the selector due to creating a single className including both the dynamic and statically-defined styles for each variation on the styles, rather than outputting two classNames and sharing one of them between each component instance.

Expected behavior:

Multiple classNames should be output from cx or the css prop on each element, where one className is the same amongst all siblings so that the & + & selector can be utilized like it can be in any number of CSS preprocessors. In the code sandbox links provided, the second and third squares should be red.

I would expect the following to happen:

const staticStyles = css`
  width: 50px;
  background-color: red;
 
  & + & {
    background-color: blue;
  }
`
const TestComponent = ({ color = 'blue' }) => <div className={cx(staticStyles, css`color: ${color};`)} />

...

<TestComponent color='purple' />
<TestComponent color='orange' />

Should render something like:

<style>
  .css-1wpld8c-TestComponent {
    width: 50px;
    background-color: red;
  }

  .css-1wpld8c-TestComponent + .css-1wpld8c-TestComponent {
    background-color: blue;
  }
</style>

<style>
  .css-klhj53jhkj-TestComponent { color: purple; }
</style>

<style>
  .css-djciui7kjb-TestComponent { color: orange; }
</style>
<div class="css-1wpld8c-TestComponent css-klhj53jhkj-TestComponent"></div>
<div class="css-1wpld8c-TestComponent css-djciui7kjb-TestComponent"></div>

Environment information:

  • react version: 16.8.6
  • emotion version: 10.0.9
@Andarist
Copy link
Member

I'm really tired right now and having hard time to focus properly on analyzing the problem at hand here - but at first sight it sounds similar to the old issue reported and maybe this explanation will help you: #743 (comment) . If not please respond that this is not the same problem and I could try to take a look at this later.

@DesignerDave
Copy link
Author

@Andarist It looks like a similar issue to the one you linked, with the exception that we're not using styled, and that where we use & + & is with other completely static styles. If we weren't composing this with a dynamic set of styles, it would work fine.

Not really looking for a workaround for this as there are other (though less optimal) selectors that can achieve a similar effect for our specific case. It's more an issue of broken behavior with composition as far as I'm concerned.

@Andarist
Copy link
Member

Seems it's actually the same issue - https://codesandbox.io/s/ecstatic-pike-0c85c .

While you don't compose dynamic interpolation, your classes are "dynamic" because they use different interpolations - the one setting border-color. Emotion flattens composed styled into a single and unique class name, so in your example you are producing 3 distinct class names and those different class names are not affected by & + &.

@DesignerDave
Copy link
Author

Understood. The problem is that this is a common use-case, and the fact that style declarations are unconditionally flattened into a single output className not only makes the & + & selector unusable with Emotion in many cases, but also means each unique className has a substantial amount of duplicated styles that wouldn't otherwise need to be duplicated.

@Andarist
Copy link
Member

but also means each unique className has a substantial amount of duplicated styles that wouldn't otherwise need to be duplicated.

True, but emotion doesn't try to minimize class names count. It aims for more predictable composition as described in https://emotion.sh/docs/composition . Without flattening it would also be true to achieve this composition model, because we would have to track insertion order and do some weird shenanigans to allow for what you are asking for (and in the meantime we would probably break dozens of other patterns). Flattening brings this unique treat onto the table with ease of its implementation.

I understand why this is surprising though and this probably should be expplained as caveat in the docs.

@DesignerDave
Copy link
Author

So that selector is broken in elements with dynamic styles, and there's no plans to fix it? That may be predictable composition from an implementation standpoint but it's not predictable behavior from the perspective of someone using the library IMO.

Not that it's an ideal implementation anyways, but the workaround suggested in the other issue simply won't work for us because we are unable to use styled due to needing to use a custom instance of Emotion:

const Container = styled.div`
  & + ${() => Container} {
 	margin-top: 20px;
  }
`

This would be an easier pill to swallow if we could interpolate Emotion-generated classNames without getting a console error, but as of now there's no way to get around this composition behavior, and we're stuck without using this selector altogether.

The other option (which we've already needed to make use of due to the className interpolation restriction) would be to assign a data- attribute to the component, and then do something like [data-my-selector] + [data-my-selector], but that's obviously less than ideal (not to mention less performant from a selector perspective).

@Andarist
Copy link
Member

Emotion indeed restricts some css patterns - ideally we would like to cover all use cases, but that's just hard and considering pros and cons of the used approach we believe that those are tradeoffs worth the received benefits.

Ofc you can always attempt to prepare a PR fixing this while preserving existing tests intact.

The workaround could be:

const Container = styled.div`
  ${({ adjacent }) => adjacent && { marginTop: 20 }}
`;

const Component = () => {
  return (
    <>
      [1, 2, 3].map((_, i) => <Container adjacent={i > 0} />)
    </>
  );
};

@DesignerDave
Copy link
Author

Thanks for the responses. I think the workaround I'm most likely to use in this instance is something like:

const styles = css`
  margin-right: 5px;

  &:last-child {
    margin-right: 0;
  }
`;

rather than

const styles = css`
  & + & {
    margin-left: 5px;
  }
`;

Obviously this is a narrow workaround that wouldn't work for many use-cases, but should be fine for this one.

If I end up with the time, I may try making a fix for this. Thanks for answering my questions!

@Andarist
Copy link
Member

Closing this, because it really doesn't seem to be solvable by emotion. It's just that & refers to something else that you have expected here as it refers to a final computed class name, rather than some kind of stable one. Because we flatten composed styles into a single class name to make things predictable (independent of insertion order) this just cannot refer to a stable class.

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

2 participants