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

Restructured the default-styles module into a wysiwyg module #14

Merged
merged 1 commit into from
Jul 21, 2015

Conversation

nathansh
Copy link

Since A) we're using normalize.css by default, and B) the _default-styles.scss module was largely misused, I've restructured this into a much more obvious form. It's much clearer now that this module is only for content output by a wysiwyg, and the number of items in the module is greatly reduced since we're using normalize. This promotes better defaults in _base.scss, moving to a more Inverted Triangle CSS pattern (https://www.youtube.com/watch?v=1OKZOV-iLj4).

@reubenmoes
Copy link
Contributor

My preference would be to style margin+styles of p,ol,ul within _wysiwyg and not _base so that you don't have to reset lists and paragraphs within every module.

not a huge fan of this in _base:

p, ul, ol {
    &:first-of-type {
        margin-top: 0; // This is because of normalize. If you Meyer you have it anyway.
    }
    &:last-of-type {
        margin-bottom: 0; // This is because of normalize. If you Meyer you have it anyway.
    }
}

What if we put something like this in _base instead?

//A selective reset
p, ul,ul,ol {
    margin: 0;
    padding: 0;
    border: 0;
    outline: 0;
    vertical-align: baseline;
}
ol, ul {
    list-style: none;
}

@igorbarbashin
Copy link
Contributor

I guess it makes sense to reset ol and ul, but not p.
Paragraphs should behave typographically correct - have margins after them, even outside of wysiwyg modules.
Actually same could be applied to ol. Cause it's not usually used for navigation items. Probably there's no reason to reset it.

@reubenmoes
Copy link
Contributor

I think adding two levels of specificity (p:first-of-type) to _base.scss is not the answer when this is most likely needed for content/wysiwyg areas.

The reason being, that if you want to add some top margin on a BEM module:

p, ul, ol {
    &:first-of-type {
        margin-top: 0; // This is because of normalize. If you Meyer you have it anyway.
    }
    &:last-of-type {
        margin-bottom: 0; // This is because of normalize. If you Meyer you have it anyway.
    }
}
//markup <div class="module">...<p class="module__intro">asdf</p>...</div>
.module {
  &__intro {
    margin: 20px 0;  //This will have top and bottom margin 0 because p:first-of-type and p:last-of-type have more specificity.
  }
}

@reubenmoes
Copy link
Contributor

Maybe we just need to add the p:first-of-type stuff into _wysiwyg and not _base ?

@nathansh
Copy link
Author

So, the idea is, we're only applying the things that are actually needed on every project, and relying on normalize for the rest. So you really don't need any specific things for p, ol, ul on every project until you actually see the design. For your point @reubenmoes, anything that is specific to just wysiwyg content and not global you'd put in _wysiwyg.scss, not _base.scss. The distinction is global/vs wysiwyg content, and that'd be a decision you'd make in build depending on the design. I think this leaves that open. I definitely don't think we should move css resetting into _base.scss though.

@nathansh
Copy link
Author

@reubenmoes have you built any projects with normalize.css yet? I'd be curious if you've encountered that issue I'm trying to deal with. Maybe we do reset the margins on p earlier and add that one just in _wysiwyg.scss, that could be a solution.

@reubenmoes
Copy link
Contributor

Redlands will be my first normalize project however I got rid of the p:first-of-type stuff because I ran into that right away. I would be all for this:
_base.scss

p,ol,ul, li {
  margin: 0;
  padding: 0;
}
ol,li {
  list-style: none;
}

_wysiwyg.scss something along this line

  p,
  ol,
  ul {
    margin-top: 1em;
    margin-bottom: 1em;
    &:first-of-type {
        margin-top: 0;
    }
    &:last-of-type {
        margin-bottom: 0; 
    }
  }

@nathansh
Copy link
Author

@reubenmoes What do you think about merging this and addressing resetting in a separate thread/pr?

@reubenmoes
Copy link
Contributor

yup

@reubenmoes
Copy link
Contributor

+1 for merge

nathansh pushed a commit that referenced this pull request Jul 21, 2015
Restructured the default-styles module into a wysiwyg module
@nathansh nathansh merged commit 61f19a8 into master Jul 21, 2015
@nathansh nathansh deleted the wysiwyg branch July 21, 2015 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants