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

Vertical rhythm #295

Merged
merged 43 commits into from Sep 16, 2015
Merged

Vertical rhythm #295

merged 43 commits into from Sep 16, 2015

Conversation

mr-mig
Copy link
Contributor

@mr-mig mr-mig commented Sep 16, 2015

Note: this PR will be merged into master and will be a huge breaking change!

Close #13
Close #101
Close #230
Close #192
Close #112

Relates to #103, #104, #108.

Mockup used: http://typecast.com/QhQQkkddGr/share/87eeaf5d967424f871d1236edad36ee3d46a85b6KrPQgjrbw

Incorporate changes from: #254 #271 #291

Changes

This PR introduces changes to the whole code base and touches almost every non-trivial component.

Font Map and fontSize function

From now on there is a font map, which incorporates all possible font sizes which should be used across components.
There are 2 axes:

  • 2D dimensions for "labels" ("labels" is a set of text-based components which are not supposed to wrap more than in 2 lines and cannot change it's size arbitrary. E.g. Text Bit is a "label".): default, tiny, small, medium, large, xlarge, xxlarge
  • Importance dimension for copy text (headers, content text): default, obscure, standout, headline, header
$fontSizes: (
  default: 1rem,      // 16px
  tiny: 0.625rem,     // 10px
  small: 0.8125rem,   // 13px
  medium: 0.9375rem,  // 15px
  large: 3.625rem,    // 58px
  xlarge: 4.5rem,     // 72px
  xxlarge: 5rem,      // 80px
  obscure: 0.75rem,   // 12px
  standout: 1.125rem, // 18px
  headline: 1.75rem,  // 28px
  header: 3rem        // 48px
);

Ideally, this map should not change.

Additionally, there is a function for getting font size based on size name, fontSize:

font-size: fontSize(small);

rhythm function

There is a function for easier adding sizes, which are multiples of $baseline:

height: rhythm(10); // translates to 15rem == 240px by default
margin-bottom: rhythm(1/2);

The usage is the same as in ruby's compass.

typeVariant mixin

Used to set up copy text properties.
Allows text to be pushed to baseline.

See details: #254

components-container mixin

Used in containers to mark holes, where other arbitrary components can go.
Uses flexbox + wrap + vertical centering by default.

Pending Refactors

  1. mint-checkbox implementation need to be reconsidered Revisit checkbox implementation #299
  2. mint-list should be transformed to container using flexbox Generic "holes" for containers with unlimited number of components in a single row #104
  3. mint-promo-box should be revisited Revisit promo-box #300
  4. Introduce inline-component/small-component Introduce inline-component/small-component #301
  5. Make positioning using flexbox Flex-box for layouting #103 - will help to deal with inline-block elements problems (descenders, repositioned baseline, baseline vs overflow:hidden)
  6. Letter spacing for uppercase text in labels/components Letter spacing for uppercase text in labels/components #302
  7. figure out a better way of triggering the rhythm guidelines Figure out a better way of triggering the rhythm guidelines #303
  8. omit $fontWeightBlack variable usage - use bolder instead omit $fontWeightBlack variable usage #304
  9. Separators should be revisited Separators should be revisited #305

Breaking Changes

Containers:

  • mint-bubble, added __hole
  • mint-box, added __hole
  • mint-content-box, added __title
  • footer lost all original styling for holes. Use mint-link and mint-text instead!

Components:

  • .mint-breadcrumb-list lost its specific text styles. Now it is more like a container - you can put arbitrary text blocks inside.

Text:

mint-text-block -> mint-text--obscure
mint-text-description -> mint-text--standout
mint-text-description--large -> mint-text--headline
mint-text-description--large + bold -> mint-header-primary--small

These things have changed it dimensions and MAY need refactoring:

mint-header-primary -> mint-text-bit
mint-header-primary--small -> mint-text-bit--small
mint-text-emphasised -> mint-text--emphasised

@@ -3,24 +3,39 @@ $include-html: false !default;
@if ($include-html) {

.mint-content-box {

&__title {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This container is supposed to have only headers. Some of them can wrap, like mint-header--secondary, hence the min-height.

@aju
Copy link
Contributor

aju commented Sep 16, 2015

👍

@@ -0,0 +1,40 @@
.mint-link {
@include remove-descenders;
cursor: pointer;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kdzwinel should this pointer go as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that one - links natively have cursor: pointer, so this setting only makes sense if we create something that should look and feel like link but should not be an <a>. Do we have such cases?

Anyway, this works on opera mini (tested in footer where we have links now), so that aspect is not an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, clear. I'll leave it as is 👍

@kdzwinel
Copy link
Contributor

Besides the small things mentioned - 👍

@vergilius
Copy link
Contributor

well done! this is huge improvement ;)
besides my quesions 👍

@aju
Copy link
Contributor

aju commented Sep 16, 2015

Lets 🐑 🇮🇹
Good job! 🍰 🎆

mr-mig added a commit that referenced this pull request Sep 16, 2015
@mr-mig mr-mig merged commit ba79249 into master Sep 16, 2015
@mr-mig mr-mig deleted the dev/vertical-rhythm branch September 16, 2015 15:07
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.

None yet

4 participants