Placeholder extend leaves empty selector #46

Closed
kristoferjoseph opened this Issue Mar 9, 2013 · 17 comments

Comments

Projects
None yet
4 participants
@kristoferjoseph
Contributor

kristoferjoseph commented Mar 9, 2013

Using placeholder extension leaves an empty selector in the output.
An example of this can be seen in the test file:
https://github.com/visionmedia/rework/blob/master/test/fixtures/extend.nested.placeholder.out.css

Expected result seems like it would be either to insert background: black; or to remove the empty selector if the generated extended selectors cover that case in the cascade.

Steps to reproduce

  1. Create a placeholder selector:

    %dark-button {
        background: black;
    }
    
    .button {
        extend: %dark-button;
    }
  2. Run rework use extend

  3. Compare output

Expected result

.button {
    background: black;
}

Actual result

.button {
    background: black;
}

.button {

}
@kristoferjoseph

This comment has been minimized.

Show comment Hide comment
@kristoferjoseph

kristoferjoseph Mar 11, 2013

Contributor

Tidy plugin?

Contributor

kristoferjoseph commented Mar 11, 2013

Tidy plugin?

@ForbesLindesay

This comment has been minimized.

Show comment Hide comment
@ForbesLindesay

ForbesLindesay Mar 11, 2013

👍 for some sort of tidy plugin. There are a few neat things we can do. The easiest is to remove empty selectors, but we also have the option of combining selectors that are consecutive and have identical selector strings. If we wanted to start digging around in the selector strings we could become really super clever.

👍 for some sort of tidy plugin. There are a few neat things we can do. The easiest is to remove empty selectors, but we also have the option of combining selectors that are consecutive and have identical selector strings. If we wanted to start digging around in the selector strings we could become really super clever.

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Mar 11, 2013

Member

nah no need for tidy plugin this is just a "bug"

Member

tj commented Mar 11, 2013

nah no need for tidy plugin this is just a "bug"

@kristoferjoseph

This comment has been minimized.

Show comment Hide comment
@kristoferjoseph

kristoferjoseph Mar 11, 2013

Contributor

OK. Where do you think the fix should be?

Contributor

kristoferjoseph commented Mar 11, 2013

OK. Where do you think the fix should be?

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Mar 11, 2013

Member

we should be able to check declarations.length around here and splice away that ruleset if it's empty, or add something to lib/utils.js that we can re-use in other plugins that remove props

Member

tj commented Mar 11, 2013

we should be able to check declarations.length around here and splice away that ruleset if it's empty, or add something to lib/utils.js that we can re-use in other plugins that remove props

@ForbesLindesay

This comment has been minimized.

Show comment Hide comment
@ForbesLindesay

ForbesLindesay Mar 11, 2013

I agree it's a bug with the existing placeholder plugin, but it would be nice if there was also a tidy plugin, for two reasons.

a) it's likely that quite a few transformations might accidentally result in empty selectors
b) It could be extended to do much more clever minification by merging selectors that are guaranteed to select the exact same items, or that have the exact same properties.

I agree it's a bug with the existing placeholder plugin, but it would be nice if there was also a tidy plugin, for two reasons.

a) it's likely that quite a few transformations might accidentally result in empty selectors
b) It could be extended to do much more clever minification by merging selectors that are guaranteed to select the exact same items, or that have the exact same properties.

@kristoferjoseph

This comment has been minimized.

Show comment Hide comment
@kristoferjoseph

kristoferjoseph Mar 11, 2013

Contributor

OK. This was where I had originally put the fix. There seems to be other processes that are leaving empty selectors as well. Utils seems like a good place to put the fix for reuse.

I do agree with @ForbesLindesay that a tidy plugin would be nice. Things like specifying property order, fixing lint errors, optimizing selectors etc. Maybe this is a feature for a later time though.

Contributor

kristoferjoseph commented Mar 11, 2013

OK. This was where I had originally put the fix. There seems to be other processes that are leaving empty selectors as well. Utils seems like a good place to put the fix for reuse.

I do agree with @ForbesLindesay that a tidy plugin would be nice. Things like specifying property order, fixing lint errors, optimizing selectors etc. Maybe this is a feature for a later time though.

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Mar 11, 2013

Member

no reason it can't be a third-party plugin

Member

tj commented Mar 11, 2013

no reason it can't be a third-party plugin

@ForbesLindesay

This comment has been minimized.

Show comment Hide comment
@ForbesLindesay

ForbesLindesay Mar 11, 2013

I'm all for more 3rd party plugins, anything to stop the corrosive effect of labelling some modules "core" and those that aren't "core" be seen as too crazy to use since they've not been stamped "core".

I'm all for more 3rd party plugins, anything to stop the corrosive effect of labelling some modules "core" and those that aren't "core" be seen as too crazy to use since they've not been stamped "core".

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Mar 11, 2013

Member

to me core plugins are:

  • well-defined behaviours that are easy to maintain
  • cover a common use-case

this would be one that isn't well-defined

Member

tj commented Mar 11, 2013

to me core plugins are:

  • well-defined behaviours that are easy to maintain
  • cover a common use-case

this would be one that isn't well-defined

@kristoferjoseph

This comment has been minimized.

Show comment Hide comment
@kristoferjoseph

kristoferjoseph Mar 11, 2013

Contributor

Would adding parameters to the tidy plugin make it more well-defined? There are a lot of existing tidy plugins.

Contributor

kristoferjoseph commented Mar 11, 2013

Would adding parameters to the tidy plugin make it more well-defined? There are a lot of existing tidy plugins.

@ForbesLindesay

This comment has been minimized.

Show comment Hide comment
@ForbesLindesay

ForbesLindesay Mar 11, 2013

No, it's not about that, it's about the fact that it has quite broad functionality. It will always be possible to tinker with it and extend it in some way. By contrast something like inline is done, and is practically locked.

No, it's not about that, it's about the fact that it has quite broad functionality. It will always be possible to tinker with it and extend it in some way. By contrast something like inline is done, and is practically locked.

@jonathanong

This comment has been minimized.

Show comment Hide comment
@jonathanong

jonathanong Mar 12, 2013

Contributor

also, unused placeholders leave rules without any selectors

%stuff {
  color: white
}

yields

 {
  color: white
}

you shouldn't be doing this anyways, but i would still consider it a bug

Contributor

jonathanong commented Mar 12, 2013

also, unused placeholders leave rules without any selectors

%stuff {
  color: white
}

yields

 {
  color: white
}

you shouldn't be doing this anyways, but i would still consider it a bug

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Mar 12, 2013

Member

@jonathanong woah yeah that's not good, can you open an issue for that?

Member

tj commented Mar 12, 2013

@jonathanong woah yeah that's not good, can you open an issue for that?

kristoferjoseph added a commit to kristoferjoseph/rework that referenced this issue Mar 13, 2013

@kristoferjoseph kristoferjoseph referenced this issue Mar 13, 2013

Merged

Issue 46 #55

@kristoferjoseph

This comment has been minimized.

Show comment Hide comment
@kristoferjoseph

kristoferjoseph Mar 13, 2013

Contributor

Got it. Thanks for defining that for me.
Are there any notes on how to make / use third party plugins with rework?

Contributor

kristoferjoseph commented Mar 13, 2013

Got it. Thanks for defining that for me.
Are there any notes on how to make / use third party plugins with rework?

@tj

This comment has been minimized.

Show comment Hide comment
@tj

tj Mar 13, 2013

Member

just the exact same thing except published as an npm module, .use(require('my-module'))

Member

tj commented Mar 13, 2013

just the exact same thing except published as an npm module, .use(require('my-module'))

@kristoferjoseph

This comment has been minimized.

Show comment Hide comment
@kristoferjoseph

kristoferjoseph Mar 14, 2013

Contributor

Very slick. Thanks.

Contributor

kristoferjoseph commented Mar 14, 2013

Very slick. Thanks.

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