csstools / postcss-short

Use advanced shorthand properties in CSS
https://jonathantneal.github.io/postcss-short
Creative Commons Zero v1.0 Universal
189 stars 7 forks source link

Break into multiple modules #3

Closed fregante closed 9 years ago

fregante commented 9 years ago

As per mandatory PostCSS Plugin Guidelines, plugins should do one thing.

Do not create multitool plugins. Few small one-purpose plugins with a plugins pack will be a better solution

postcss-short already overlaps with several other plugins:

https://github.com/jonathantneal/postcss-pseudo-class-enter https://github.com/seaneking/postcss-position https://github.com/postcss/postcss-size https://github.com/postcss/postcss-focus https://github.com/ben-eb/postcss-minify-font-weight https://github.com/ben-eb/postcss-colormin

As it is, only the partial shorthands and the text shorthand are unique to postcss-short. Perhaps they could be extracted as postcss-partial-shorthand and postcss-text-property?

ai commented 9 years ago

Yeap, plugin pack is better way to handle it.

ai commented 9 years ago

@jonathantneal what do you think to became a plugins pack like a cssnext or cssnano?

I want to move from many small plugins to your pack in my company :).

jonathantneal commented 9 years ago

I agree with you and @bfred-it, @ai. This needs to be made into a pack.

While you did not mention it, the asterisk (*) shorthand is the most unique feature in this plugin. To integrate it with postcss-size, I would either need to A. drop support for asterisks within size, B. run after postcss-size and cleanup asterisks on width and height, or C. duplicate postcss-size functionality within shorthands.

How I integrate with postcss-size will determine how I handle position: absolute N E S W;.

I may be able to have this done over the next week. Please forgive my mistakes. This was my first postcss plugin. Thanks for filing this issue.

ai commented 9 years ago

If you want to change postcss-size behaviour, just send a PR to it :)

jonathantneal commented 9 years ago

I have published css-font-weight-names, doing my best to follow in the footsteps of css-color-names.

fregante commented 9 years ago

Isn't css-font-weight-names a duplicate of the previously-linked postcss-minify-font-weight? (edit: Yours has more weights but perhaps these changes can be applied to that repo instead)

So would css-color-names be to postcss-colormin

jonathantneal commented 9 years ago

@bfred-it, postcss-minifiy-font-weight is a postcss plugin that minifies 2 font weights, normal and bold, whereas css-font-weight-names is a npm module that produces an object of CSS Fonts Module Level 3 font weights, similar to css-color-names.

postcss-minify-font-weight could be updated to support the other 21 font weight names.

fregante commented 9 years ago

Yes, sorry, I had made an edit to the comment. The intent of the plugins is similar

jonathantneal commented 9 years ago

The intent of supporting many font weights was unique to postcss-short, but I agree that this functionality should be moved into postcss-minify-font-weights.

Note: This means that postcss-minify-font-weights would incidentally do more than minify font weights, since its dependency would support more font weights. This is similar to how postcss-colormin incidentally does more than minify colors, since its dependency supports rebeccapurple.

Moving forward, I should probably fork postcss-minify-font-weight, implement css-font-weight-names, and send a pull request to @ben-eb. I’m giving him the heads up here, in case he has any questions or concerns.

fregante commented 9 years ago

:+1:

ben-eb commented 9 years ago

@jonathantneal Are these keywords usable by browsers? If not then it shouldn't be a responsibility of a processor that is intended to optimise a legal value in CSS to one that is shorter; this syntax sugar should be its own module.

fregante commented 9 years ago

It doesn't look like they are: http://www.w3.org/TR/css3-fonts/#font-weight-prop

My bad, then.

Edit: By the way rebeccapurple is now in the spec, so technically it's no different than red (which should probably stay red, not become #ff000)

jonathantneal commented 9 years ago

@bfred-it , rebeccapurple being in spec does make a difference, as these font weight names are not in a specification exactly. I say exactly because one of my sources is the spec you just referenced:

http://www.w3.org/TR/css3-fonts/#font-weight-prop

100 to 900

These values form an ordered sequence, where each number indicates a weight that is at least as dark as its predecessor. These roughly correspond to the commonly used weight names below:

100 - Thin 200 - Extra Light (Ultra Light) 300 - Light 400 - Normal 500 - Medium 600 - Semi Bold (Demi Bold) 700 - Bold 800 - Extra Bold (Ultra Bold) 900 - Black (Heavy)

@ben-eb, moving forward, would I create a postcss plugin that specifically processes these non-spec font-weights, and then, if and when they are added to a spec, remove the plugin, and make a pull request to yours?

ben-eb commented 9 years ago

@jonathantneal Yep, I am happy to include conversions for legal values, if and when that happens.

TrySound commented 9 years ago

@ben-eb I think postcss-font-weights is the nice idea for aliases

jonathantneal commented 9 years ago

I have updated the plugin to focus only on adding or extending shorthand properties in CSS. Gone is :enter, :after, etc. Gone is color and font-weight transformation. Also, I’m now following the postcss plugin boilerplate conventions, and I have a passing Travis build. I do not believe a plugin pack is needed to add font-weight or :enter back in.

fregante commented 9 years ago

Change short to only handle asterisk values (see below for size integration)

It doesn't only handle that yet; it's including colors, font weights, size, and text… right? Am I missing something?

jonathantneal commented 9 years ago

This is now a bonafide plugin pack.

fregante commented 9 years ago

:clap: :tada: