yui / skinbuilder

A tool for creating skins for YUI widgets
Other
30 stars 12 forks source link

Generated CSS doesn't match preview #19

Closed mrchief closed 11 years ago

mrchief commented 11 years ago

As per preview and my chosen settings, this should change the background color of the page to a dark one. But as you can see, there is no corresponding body declaration.

The button colors also don't match... http://jsbin.com/osecov/1/

image

Tried pasting the CSS as well using the link...

http://yui.github.io/skinbuilder/?mode=pureindex.html?opt=mine,3175b9,545454,1,1,10,1.5&h=0,-30,60&n=0,-30,75&l=0,-30,80&b=0,-30,90&mode=pure

jconniff commented 11 years ago

@mrchief Thanks for the image and the detail to reproduce the issue. While developing Skin Builder we considered the possibility that the immediate container of UI elements might not always the a body tag. For example, on a given page, some UI controls may be in a page header area, a side panel, or different sections. These sections frequently have different background colors. We also knew that we wouldn't know what the CSS selector would be for these sections. Sections with different background colors would likely require a different set of CSS entirely. For these reasons we didn't include a rule for container background color in the CSS output.

What we meant to do, but must have forgotten is to include in the CSS output, a commented out rule for body or background selector with the color chosen that could be un-commented or customized. I'll add this as feature request.

In your opinion, would you rather have had the uncommented rule... body {background-color: #454545;} knowing that in some cases where this CSS was retrofitted into an existing page, it would clobber the existing page background?

Your other comment, "The button colors also don't match" is due to the fact that the color the user chooses for the button color is the "Primary", or most dominant button color. Any Pure buttons you want to have that color need to be done this way [begin quote from Pure docs]

Primary Buttons

To indicate that a button represents a primary action, add the pure-button-primary classname alongside pure-button.

<a class="pure-button pure-button-primary" href="#">A Primary Button</a>
<button class="pure-button pure-button-primary">A Primary Button</button> 
mrchief commented 11 years ago

Thanks for asking my opinion!

I'm kinda divided on this. but I'm leaning towards having the uncommented rule and here's why:

I think that majority of people would go for whole page/site overhaul rather than retrofitting just parts of the page (I'm thinking bootstrap/foundation users here). Even for other people, retrofitting parts is might be more involved (especially when CSS rules depend on hierarchy to work properly) and so that type of usage should be lower. So for this, adding a CSS class that targets the body wouldn't hurt.

Secondly, even if you imagine the worst case where the entire body color changes because they imported this style sheet, it takes a heartbeat to figure out the real issue and disable/take out the body selector. On the other hand, if you put it commented, it becomes less obvious as to why the body color is not getting applied and there are no clues to let the user know that - oh wait, there's a commented rule sitting for you to take action on.

Based on my experience I don't think people would be mixing bootstrap (or something else) with pure on their layout. Its gonna be all or none kind of decision. Even if they do, I'm fairly confident that those won't be the majority of the user base. Its cool that Pure was created keeping those in mind though!

Yeah, the button part I realized afterwards.

jconniff commented 11 years ago

@mrchief Good arguments in favor of uncommented body tag rule. I'm going to kick these ideas around to a few other people. thanks for the thoughtful feedback.

jconniff commented 11 years ago

From msweeney: Yeah, I guess it comes down to which is easier to figure out. I like his point about the BODY background being obvious and easy to fix, versus trying to figure out why things don't look right.

Duke3D commented 11 years ago

+1 I concur with having the rule in effect instead of commented out - much easier workflow. In general, I do not think it makes sense to produce commented rules. Turn them all on, let the user see the issue and do their own commenting to resolve any conflict with any pre-existing rules that experienced an unexpected/undesired change.

No one expects to find rendered but commented code when trying to solve an issue. They look instead at the developer tools and for highlighted specificity / override issues.
Great work!

jconniff commented 11 years ago

@mrchief @Duke3D This is fixed with commit 02816a77.

body {
    background-color: [the curent background color];
}

appears un-commented in the copyable CSS textarea js/app.js line 323

Thanks for your input and good thinking on this folks.