Adding more list variables for flexibility #4950

Merged
merged 2 commits into from Apr 25, 2014

Conversation

Projects
None yet
8 participants
@daigofuji
Contributor

daigofuji commented Apr 10, 2014

Hi there, love Foundation! First time pull request here.

I love that you have body-* style for font option for general and now paragraph-* style for <p> tag, and was using them, but encountered an issue where list items inherited paragraph-* and not body-*.

This change will add flexibility by either keeping the current setup or overriding it.

Thanks for your consideration.

@tbranyen

This comment has been minimized.

Show comment Hide comment
@tbranyen

tbranyen Apr 10, 2014

I too would like see this merged. Consistency 👍

I too would like see this merged. Consistency 👍

@rafibomb rafibomb added this to the 5.3 milestone Apr 14, 2014

@zslabs

This comment has been minimized.

Show comment Hide comment
@zslabs

zslabs Apr 18, 2014

Contributor

I completely understand the ability to customize these values; which is great, but to me this just seems like extra bloat (for the final compilation). Shouldn't we be comparing these types of variables to things like $body-font-family and $rem-base instead of continuing to re-write styles that aren't needed; they'd just fall up the tree and inherit from the defaults.

I'm not seeing normalize.css dictate anything here that would force us to re-instantiate these values, so why output them?

This isn't necessarily about this PR specifically, but something to think about for the entire framework.

Take the following for example:

body {
    font-family: Arial;
}
p {
    font-family: Arial;
}

This is in essence what we currently have going on; why though? Here's what I'm thinking would make sense:

p {
    @if $paragraph-font-family != $body-font-family {
        font-family: $paragraph-font-family;
    }
}

Checking to see if the values are different before actually writing them out. Smaller CSS files is really the end-goal here. Thanks.

Contributor

zslabs commented Apr 18, 2014

I completely understand the ability to customize these values; which is great, but to me this just seems like extra bloat (for the final compilation). Shouldn't we be comparing these types of variables to things like $body-font-family and $rem-base instead of continuing to re-write styles that aren't needed; they'd just fall up the tree and inherit from the defaults.

I'm not seeing normalize.css dictate anything here that would force us to re-instantiate these values, so why output them?

This isn't necessarily about this PR specifically, but something to think about for the entire framework.

Take the following for example:

body {
    font-family: Arial;
}
p {
    font-family: Arial;
}

This is in essence what we currently have going on; why though? Here's what I'm thinking would make sense:

p {
    @if $paragraph-font-family != $body-font-family {
        font-family: $paragraph-font-family;
    }
}

Checking to see if the values are different before actually writing them out. Smaller CSS files is really the end-goal here. Thanks.

@MattSurabian

This comment has been minimized.

Show comment Hide comment
@MattSurabian

MattSurabian Apr 18, 2014

I get what you're saying @zslabs but this approach of relying on styles falling up the tree can make it hard to develop and test individual pieces in isolation. Plus I'm not really seeing the issue with larger CSS files; a more flexible configuration seems like a greater benefit. Production CSS should be getting compressed and gzipped anyway, no?

I get what you're saying @zslabs but this approach of relying on styles falling up the tree can make it hard to develop and test individual pieces in isolation. Plus I'm not really seeing the issue with larger CSS files; a more flexible configuration seems like a greater benefit. Production CSS should be getting compressed and gzipped anyway, no?

@zslabs

This comment has been minimized.

Show comment Hide comment
@zslabs

zslabs Apr 18, 2014

Contributor

Of course it gets minified, wasn't saying it doesn't. Was simply stating that could be a potential area that could be optimized, but do see isolation benefits - guess it more depends on not introducing a level of complexity that makes the framework less usable. I'm almost thinking of what this would do in the same methodology as grunt-uncss does for overall styles; but at a much more granular level within the framework itself.

  • Zach Schnackel

On Apr 18, 2014, at 8:01 AM, Matthew Surabian notifications@github.com wrote:

I get what you're saying @zslabs but this approach of relying on styles falling up the tree can make it hard to develop and test individual pieces in isolation. Plus I'm not really seeing the issue with larger CSS files; a more flexible configuration seems like a greater benefit. Production CSS should be getting compressed and gzipped anyway, no?


Reply to this email directly or view it on GitHub.

Contributor

zslabs commented Apr 18, 2014

Of course it gets minified, wasn't saying it doesn't. Was simply stating that could be a potential area that could be optimized, but do see isolation benefits - guess it more depends on not introducing a level of complexity that makes the framework less usable. I'm almost thinking of what this would do in the same methodology as grunt-uncss does for overall styles; but at a much more granular level within the framework itself.

  • Zach Schnackel

On Apr 18, 2014, at 8:01 AM, Matthew Surabian notifications@github.com wrote:

I get what you're saying @zslabs but this approach of relying on styles falling up the tree can make it hard to develop and test individual pieces in isolation. Plus I'm not really seeing the issue with larger CSS files; a more flexible configuration seems like a greater benefit. Production CSS should be getting compressed and gzipped anyway, no?


Reply to this email directly or view it on GitHub.

@tbranyen

This comment has been minimized.

Show comment Hide comment
@tbranyen

tbranyen Apr 18, 2014

For this specific PR, it changes variables for lists to be more specific. Using paragraph styling inside of lists makes no sense and is super inconsistent.

@zslabs I'm all in favor of doing this within the framework itself. Something like grunt-uncss is neat as a concept, but probably not something I'd ship with a production quality framework.

For this specific PR, it changes variables for lists to be more specific. Using paragraph styling inside of lists makes no sense and is super inconsistent.

@zslabs I'm all in favor of doing this within the framework itself. Something like grunt-uncss is neat as a concept, but probably not something I'd ship with a production quality framework.

@daigofuji

This comment has been minimized.

Show comment Hide comment
@daigofuji

daigofuji Apr 18, 2014

Contributor

Hi,
@zslabs, I see your point. But the reason for this PR was that I did not expect <li>'s to change font when I changed $paragraph-font-family I only expected the p to change -- but by doing so, it overridden li element's font-family (which is already in output css, so this PR will not change the total numbers of lines in the output).
Another way to do it is do this outside of framework, but since $paragraph-* options are already available, I wanted to use the feature without the unexpected side-effect. It seems that list styles are the only thing that are effected by paragraph changes in foundation.
Thanks for your consideration.

Contributor

daigofuji commented Apr 18, 2014

Hi,
@zslabs, I see your point. But the reason for this PR was that I did not expect <li>'s to change font when I changed $paragraph-font-family I only expected the p to change -- but by doing so, it overridden li element's font-family (which is already in output css, so this PR will not change the total numbers of lines in the output).
Another way to do it is do this outside of framework, but since $paragraph-* options are already available, I wanted to use the feature without the unexpected side-effect. It seems that list styles are the only thing that are effected by paragraph changes in foundation.
Thanks for your consideration.

@ctalkington

This comment has been minimized.

Show comment Hide comment
@ctalkington

ctalkington Apr 19, 2014

Contributor

PR should also add to the settings file. otherwise +1

Contributor

ctalkington commented Apr 19, 2014

PR should also add to the settings file. otherwise +1

@daigofuji

This comment has been minimized.

Show comment Hide comment
@daigofuji

daigofuji Apr 19, 2014

Contributor

@ctalkington Thanks for the suggestion. Added the same changes to _settings.css

Contributor

daigofuji commented Apr 19, 2014

@ctalkington Thanks for the suggestion. Added the same changes to _settings.css

@smileyj68

This comment has been minimized.

Show comment Hide comment
@smileyj68

smileyj68 Apr 25, 2014

I'll buy this. Thanks guys.

I'll buy this. Thanks guys.

@smileyj68 smileyj68 closed this Apr 25, 2014

@smileyj68 smileyj68 reopened this Apr 25, 2014

@smileyj68

This comment has been minimized.

Show comment Hide comment
@smileyj68

smileyj68 Apr 25, 2014

And then hit the wrong button. Derp.

And then hit the wrong button. Derp.

smileyj68 pushed a commit that referenced this pull request Apr 25, 2014

Jonathan Smiley
Merge pull request #4950 from daigofuji/list-style
Adding more list variables for flexibility

@smileyj68 smileyj68 merged commit b0f2023 into zurb:master Apr 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment