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

Type-box mixin #13

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Type-box mixin #13

wants to merge 1 commit into from

Conversation

tbredin
Copy link
Contributor

@tbredin tbredin commented Apr 5, 2016

Adds ability to set box-like items in content flow without upsetting baseline. Very basic right now, needs:

  • better unit handling (allow parameters to be set in px, rem, or baseline units)
  • tests
  • bug fixes for small sizes, where it doesn't always work as expected

Is direct fix for #4, but might also help solve #5

- needs better unit handling
- needs tests
- still buggy at small sizes
@tbredin tbredin mentioned this pull request Apr 5, 2016
@ao2
Copy link

ao2 commented Jul 12, 2017

Hi,

first of all thanks for this, I think this is an essential feature.

It looks like that the padding calculation is off tho, at least when borders are specified; to have the text inside the box actually on the baseline I need this change:

diff --git a/megatype/_box.scss b/megatype/_box.scss
index 4511e30..eab4f93 100644
--- a/megatype/_box.scss
+++ b/megatype/_box.scss
@@ -31,7 +31,7 @@
         padding: $padding;
         $total: $padding + $line-height * $size;
     } @else {
-        padding: $padding - $border /2;
+        padding: $padding - $border;
         border-width: rem-to-px($border);
         $total: $padding + $line-height * $size;
     }

Plus, in the current implementation the $total calculation could be factored out of the condition, as it's the same in both branches.

Finally I noticed that when passing $size in rems it is important to apply type-box() in a way consistent with $current-rootsize because the code does use rem-to-px() and px-to-rem().

This could not be obvious to a new megatype user (I wasn't to me), so a comment could be useful to stress to pass rem units only inside a media or all-breakpoints mixin.

Otherwise, when the breakpoint changes the text could go off the baseline.

The example code could also show that:

diff --git a/megatype/_box.scss b/megatype/_box.scss
index 4511e30..e4ea1de 100644
--- a/megatype/_box.scss
+++ b/megatype/_box.scss
@@ -61,7 +61,9 @@
 //     font-family: font-family-of($sans);
 //     background-color: tomato;

-//     @include type-box(1rem, 1, 0.5rem, 1rem);
+//     @include all-breakpoints {
+//        @include type-box(1rem, 1, 0.5rem, 1rem);
+//     }

 //     &,
 //     &:hover,
@@ -79,8 +81,10 @@
 // .button-outline {
 //     background-color: transparent;
 //     color: tomato;
-//     @include type-box(1rem, 1, 0.5rem, 1rem, 1px);
-//     border-style: solid;
+//     @include all-breakpoints {
+//         @include type-box(1rem, 1, 0.5rem, 1rem, 1px);
+//         border-style: solid;
+//     }

 //     &:hover,
 //     &:focus {

@ao2
Copy link

ao2 commented Jul 13, 2017

Hi,

I think I could work on "better unit handling (allow parameters to be set in px, rem, or baseline units)" myself to try to move this forward to have the functionality in better shape for a stable release.

However I don't feel confident working on the typographic details, for instance noticed that nested boxes do not always align to the baseline, and it is not clear to me if a type-box() call is supposed to be always accompanied by a typeset() definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants