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

fix self-referential behavior in combo selectors #2071

Merged
merged 8 commits into from Oct 8, 2018

Conversation

Projects
None yet
4 participants
@probablyup
Contributor

probablyup commented Oct 7, 2018

When a component refers to itself with the ampersand placeholder the behavior today is a bit inconsistent and won't work as intended if your component has dynamic styles.

For example:

const Comp = styled.div`
  color: ${p => p.color};

  & + & {
    margin-left: 10px;
  }
`;

<div>
  <Comp color="red">Hello</Comp>
  <Comp color="blue">World</Comp>
</div>

// renders
// <div>
//   <div class="sc-a a">Hello</div>
//   <div class="sc-a b">Hello</div>
// </div>

The second instance of Comp will not get the margin-left because a different className is generated due to the different color prop, and today ampersand refers to the current styling context className.

It effectively looks like this:

// Comp -> .sc-a
// color: red -> .a
// color: blue -> .b

.sc-a {}
.a { color: red }
.a + .a { margin-left: 10px }
.b { color: blue }
.b + .b { margin-left: 10px }

Note how the combo is .a + .a and .b + .b, so only two adjacent components with the exact same styling props would trigger a match. This is a surprising behavior and makes compositions more troublesome.

The solution is to treat the second & as a reference back to the component itself (.sc-a), not the style of the component.

Then the styles look like this:

// Comp -> .sc-a
// color: red -> .a
// color: blue -> .b

.sc-a {}
.a { color: red }
.a + .sc-a { margin-left: 10px }
.b { color: blue }
.b + .sc-a { margin-left: 10px }

And any Comp next to a Comp will receive the margin-left. Note that the reason the first ampersand isn't also switched to the static component class is that will break style isolation.

before:

& + & -> .b + .b
&[disabled] { & + & } -> .b[disabled] + .b[disabled]

after:

& + & -> .b + .sc-a
&[disabled] { & + & } -> .b[disabled] + .sc-a[disabled]

fixes #784

fix self-referential behavior in combo selectors
before:

& + & -> .b + .b

after:

& + & -> .b + .sc-a

@probablyup probablyup requested review from geelen, kitten, imbhargav5 and mxstbr Oct 7, 2018

probablyup added some commits Oct 7, 2018

cover the nested selector use case
in a nested scenario, it should refer to the main component but
also acquire any selector modifiers tacked on to the amp

@probablyup probablyup force-pushed the modify-amp-selector branch from 13c666c to 73af3ce Oct 7, 2018

@imbhargav5

Looks solid!

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

styled-components@4.0.0-beta.10-4 if anyone wants to help test. Can also hack on the "after" sandbox in OP

@imbhargav5

This comment has been minimized.

Member

imbhargav5 commented Oct 8, 2018

Perhaps we can add a snapshot test for a combination of ~ & + or something just to be sure and also for &&&. This is an example of what I had in mind. A lot of people like &&& haha. 😀. It's probably being covered somewhere, and is probably obvious to work, but think this will make it robust from breaking later.

const Wrapper = s.div`
  &&&{
    h2{
      background: red;
    }
  }
  & + & { 
    background : blue;
  }
  & + & ~ &{ 
    background : yellow;
  }
`;
@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

@imbhargav5 sure, can you add an example for me to your comment and I'll put it in the PR?

@probablyup probablyup force-pushed the modify-amp-selector branch from 4bd3981 to c8b97cd Oct 8, 2018

@imbhargav5

This comment has been minimized.

Member

imbhargav5 commented Oct 8, 2018

@probablyup Just did

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

@imbhargav5

This comment has been minimized.

Member

imbhargav5 commented Oct 8, 2018

Awesome.

probablyup added some commits Oct 8, 2018

@mxstbr

mxstbr approved these changes Oct 8, 2018

If it works I guess LGTM, but damn I'm afraid of that regex having unintended edge cases 😕

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

@kitten any issues or tests I should add to the suite?

@kitten

This comment has been minimized.

Member

kitten commented Oct 8, 2018

@probablyup looks solid to me; the only thing we could do differently is to actually add it as a stylis middleware, to its selector input transform function.

@probablyup probablyup merged commit bb6194e into develop Oct 8, 2018

1 of 2 checks passed

bundlesize Checking output size...
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@probablyup probablyup deleted the modify-amp-selector branch Oct 8, 2018

dumbNickname added a commit to dumbNickname/jest-styled-components that referenced this pull request Oct 25, 2018

dumbNickname added a commit to dumbNickname/jest-styled-components that referenced this pull request Oct 31, 2018

MicheleBertoli added a commit to styled-components/jest-styled-components that referenced this pull request Oct 31, 2018

Adjust toHaveStyleRule method to use proper base component name (chan… (
#201)

* Adjust toHaveStyleRule method to use proper base component name (changed in commit: styled-components/styled-components@1c19edc#diff-8a1d0988ee5ae37265b4647fa41def2a), disabled calling lifecycle methods for enzyme shallow rendering  - unfortunately this would need to be fixed in tests that use the library (no better solution yet).

* Fix self-ref issue and try to remain compatible with all other cases. For the delf ref issue read more here: styled-components/styled-components#1275 or take a look at initial PR: styled-components/styled-components#2071

* Remove tests that got outdated to to changes in styled-components implementation

MicheleBertoli added a commit to styled-components/jest-styled-components that referenced this pull request Nov 10, 2018

Adjust toHaveStyleRule method to use proper base component name (chan… (
#201)

* Adjust toHaveStyleRule method to use proper base component name (changed in commit: styled-components/styled-components@1c19edc#diff-8a1d0988ee5ae37265b4647fa41def2a), disabled calling lifecycle methods for enzyme shallow rendering  - unfortunately this would need to be fixed in tests that use the library (no better solution yet).

* Fix self-ref issue and try to remain compatible with all other cases. For the delf ref issue read more here: styled-components/styled-components#1275 or take a look at initial PR: styled-components/styled-components#2071

* Remove tests that got outdated to to changes in styled-components implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment