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

Remove redundant declarations from output #125

Open
rosejn opened this issue Nov 18, 2016 · 4 comments
Open

Remove redundant declarations from output #125

rosejn opened this issue Nov 18, 2016 · 4 comments
Milestone

Comments

@rosejn
Copy link

rosejn commented Nov 18, 2016

When the same attribute is included multiple times in a CSS clause it seems like they should be merge rather than concatted, which produces the same attribute twice. Would this make sense as the default behavior, rather than us needing to call merge manually as is shown below to not generate two color attributes?

think.ui.repl=> (require '[garden.core :refer [css]])
nil
think.ui.repl=> (css [[:body {:color :red} {:color :blue}]])
"body {\n color: red;\n color: blue;\n}"

think.ui.repl=> (css [[:body (merge {:color :red} {:color :blue})]])
"body {\n color: blue;\n}"

@WorldsEndless
Copy link
Collaborator

I only wonder how important this might be; if it were style-sheet wide it would make sense (those common cases where something is redefined 500 lines later in the file), but might also be a real pain to implement in that case. Would it be better to just leave developers to take responsibility for their redundancy?

@rosejn
Copy link
Author

rosejn commented Nov 18, 2016

Yeah, not sure. In our case it's because we've started importing some large CSS files into garden as a bunch of functions we can mixin. This means we have style definitions along these lines:

[:ul.product-list
(mdl/vertical-list)
{:margin-left (em 1)}]

And so if the mixin vertical-list function has a margin-left it will now result in a duplicate attribute. Without knowing about every attribute produced by a mixin you can't really know in advance whether you need to (merge ...) or not.

@noprompt
Copy link
Owner

This is something that could be added to the compiler. Let me think about it. A new compiler is about to land in the 2.0.0 branch (that uses and actual AST) and this would be trivial to implement and offer as optional behavior.

@noprompt noprompt changed the title Merge by default... maybe? Remove redundant declarations from output Nov 21, 2016
@noprompt noprompt added this to the v2.0.0 milestone Nov 21, 2016
@neatonk
Copy link

neatonk commented Mar 28, 2017

The current behavior can be used to provide a fallback declaration for older browsers. As an example, I might include two width declarations in order to provide a px fallback when using viewport units. A browser that does not recognize vw units will ignore the second declaration and use the first declaration instead.

[:.myclass
 {:width (px 100}}
 {:width (vw 10}]
.myclass {
  width: 100px;
  width: 10vw;
}

If you decide to implement this optimization, you can continue to support the use case above by making the optimization optional or by providing a means to protect the fallback declarations from removal.

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

4 participants