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

fix nested selector with custom selectors #298

Merged
merged 1 commit into from
Aug 16, 2016
Merged

fix nested selector with custom selectors #298

merged 1 commit into from
Aug 16, 2016

Conversation

yordis
Copy link
Contributor

@yordis yordis commented Jul 24, 2016

@MoOx I notice the comment about the order matters. I change the order and all the test pass. I tried to add some testing but I couldn't understand how it works. I need some help with that because I can't use @custom-selector inside the nested example.

fixes #297

@MoOx
Copy link
Owner

MoOx commented Jul 28, 2016

Hi @yordis. Thanks for this PR. Tests will be great, especially to avoid regressions in the future. Do not hesitate to try the gitter https://gitter.im/MoOx/postcss-cssnext so we can help you working on the tests. Otherwise, I can try to help you from here. What do you want to know about testing? There is a command (npm test) to run all tests, that are defined in src/__tests__/.

@@ -41,6 +41,13 @@ const testFeature = function(
// enable only the one we want to test...
options.features[feature] = true

if (featureDep[feature]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the nesting feature is using custom-selector we need to enable customSelectors feature as well.

@MoOx
Copy link
Owner

MoOx commented Aug 6, 2016

This is not how we should test this. We should more rely on a complete tests with all features to ensure they works together.

@yordis
Copy link
Contributor Author

yordis commented Aug 6, 2016

@MoOx sorry I asked for direction but I didn't get that much help. What should I do then?

@MoOx
Copy link
Owner

MoOx commented Aug 6, 2016

I don't have lots of time to help sorry. I am busy with some other projects.
Maybe the best thing to do is to add a new test file in https://github.com/MoOx/postcss-cssnext/tree/master/src/__tests__ where you can put a big example with all features to be sure they are all working (you can probably take the playground example and adjust for your specific bug fix). Hope that helps.

@yordis
Copy link
Contributor Author

yordis commented Aug 11, 2016

@MoOx I finished ❤️ . Is it right this way? Basically that file is kind of a regression file.

@custom-selector :--button .button;

:--button {
&--active {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this is ok according to nest spec. Can you triple check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using a nested style rule, one must be able to refer to the elements matched by the parent rule; that is, after all, the entire point of nesting.

This change fix the output to get the expected result from the documentation.

Copy link
Contributor Author

@yordis yordis Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From this example with the current implementation

Code base: https://github.com/caribbeanpirates/boa/blob/master/src/button/button.css

@custom-selector :--boa-button .boa-button;
:--boa-button {
  &--active {
    color: red;
  }
}

Console output

postcss-custom-selectors: /boa/src/button/button.css:70:3: The selector ':--boa-button--active' is undefined

Output

:--boa-button--active {
  color: red
}

As you notice is not the correct output at all

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I read in spec http://tabatkins.github.io/specs/css-nesting/#direct &--active selector is invalid. Not sure why postcss-nesting transform this. poke @jonathantneal

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue csstools/postcss-nesting#17 confirm that this behavior should not work.

Copy link
Contributor Author

@yordis yordis Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MoOx and that's fine but the spec need to take in consideration the de-sugaring of the custom selector. That's why I think we need some feedback from @tabatkins about that.

I understand that use matches but it should take in consideration the custom selectors. Either way, specs are made to be change in the future as need it, I hope!

Copy link

@tabatkins tabatkins Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given:

@custom-selector :--blah .blah p;

:--blah {
  &--wat { ... }
}

this desugars to:

--wat:matches(:--blah) { ... }

exactly like the spec says. ^_^ (Note that --wat is a valid ident, which is interpreted by Selectors as a type selector.)

You can then desugar the custom selector as well, getting:

--wat:matches(.blah p) { ... }

As it so happens, this is guaranteed to match nothing, since no element has both "p" and "--wat" as its tagname.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabatkins let's remove for a moment the --wat. How should be the output of this

@custom-selector :--blah .blah p;

:--blah {
  & span { ... }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, per spec that desugars to:

:matches(.blah p) span { ... }

Or, simplified (because the :matches() doesn't do anything useful there):

.blah p span { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tabatkins sorry that took me so long I just wanted to triple check as @MoOx asked.

@yordis
Copy link
Contributor Author

yordis commented Aug 16, 2016

@MoOx indeed, I think that my fix is not a valid point.

The issue is that I am using nesting in a wrong way with I guess I can't use it as a string interpolation which make sense from CSS point of view.

Also is a valid point to have custom selector after nesting because this could be something like :matches(:--button) span { color: blue; } in the future.

I will fix the test, I will change the order back to the normal but I will the PR so we can use it in the future for regression test if you don't mind

@yordis
Copy link
Contributor Author

yordis commented Aug 16, 2016

@MoOx I hope in the future we have such of "string interpolation" or "selector interpolation" in CSS 😃

Check the PR, I just reversed back but I left the regression test. Thank you very much for the patient and help on this

@MoOx MoOx merged commit 90d38ff into MoOx:master Aug 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nested selectors doesn't with custom selectors
3 participants