Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Adding more list variables for flexibility #4950

Merged
merged 2 commits into from

8 participants

@daigofuji

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

I too would like see this merged. Consistency :+1:

@rafibomb rafibomb added this to the 5.3 milestone
@zurbjeff zurbjeff was assigned by rafibomb
@zurbjeff zurbjeff was unassigned by rafibomb
@smileyj68 smileyj68 was assigned by rafibomb
@zslabs

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

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
@tbranyen

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

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

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

@daigofuji

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

@smileyj68

I'll buy this. Thanks guys.

@smileyj68 smileyj68 closed this
@smileyj68 smileyj68 reopened this
@smileyj68

And then hit the wrong button. Derp.

@smileyj68 smileyj68 merged commit b0f2023 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 10, 2014
  1. @daigofuji
Commits on Apr 22, 2014
  1. @daigofuji
This page is out of date. Refresh to see the latest.
View
4 scss/foundation/_settings.scss
@@ -174,6 +174,10 @@
// $hr-margin: rem-calc(20);
// We use these to style lists
+// $list-font-family: $paragraph-font-family;
+// $list-font-size: $paragraph-font-size;
+// $list-line-height: $paragraph-line-height;
+// $list-margin-bottom: $paragraph-margin-bottom;
// $list-style-position: outside;
// $list-side-margin: 1.1rem;
// $list-ordered-side-margin: 1.4rem;
View
12 scss/foundation/components/_type.scss
@@ -63,6 +63,10 @@ $hr-border-color: #ddd !default;
$hr-margin: rem-calc(20) !default;
// We use these to style lists
+$list-font-family: $paragraph-font-family !default;
+$list-font-size: $paragraph-font-size !default;
+$list-line-height: $paragraph-line-height !default;
+$list-margin-bottom: $paragraph-margin-bottom !default;
$list-style-position: outside !default;
$list-side-margin: 1.1rem !default;
$list-ordered-side-margin: 1.4rem !default;
@@ -288,11 +292,11 @@ $align-class-breakpoints:
ul,
ol,
dl {
- font-size: $paragraph-font-size;
- line-height: $paragraph-line-height;
- margin-bottom: $paragraph-margin-bottom;
+ font-size: $list-font-size;
+ line-height: $list-line-height;
+ margin-bottom: $list-margin-bottom;
list-style-position: $list-style-position;
- font-family: $paragraph-font-family;
+ font-family: $list-font-family;
}
ul {
Something went wrong with that request. Please try again.