Remove (or explain) reset-baseline in vertical rhythym parital #780

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@JohnAlbin

The attached commit reverts commit c92766b.

I looked at the new reset-baseline() mixin in Compass 0.12 and I'm completely at a loss for why it was added. @scottdavis, can you share your thoughts?

Firstly, the if($relative-font-sizing, $base-font-size, $base-font-size) has to be a typo, right?

Also, the name seems misleading. reset-baseline should set the font size and the line height, I would think. establish-baseline() sets the font size and the line height. But reset-baseline() only sets the line height.

Finally, since $relative-font-sizing defaults to true and $font-unit defaults to 1em, the reset-baseline() mixin sets a line height that is relative to… the element's font size. And if the element's font size isn't the base font size, then the line-height has been set to a value which is NOT conforming to the base line height/vertical rhythm.

It appears you can only use this mixin if the element happens to be using a font size equal to $base-font-size.

@scottdavis
Compass member

I had a need in my application to reset the baseline from within the cascade on an element that was styled above it in the tree. I wanted to only reset the line height not the font size to maintain correct spacing.

It was on an H3 that was being modified outside of the scope of the rhythm in a non content area. (Its hard to explane without showing you)

also yes that was a typo maybe it does need to be renamed to reset-line-height? im open for suggestions, but there is a use case for this so I don't want it removed.

@JohnAlbin

I wanted to only reset the line height not the font size to maintain correct spacing.

I'm concerned it doesn't actually reset the line height. Because it is relative to the element's font size. Maybe it worked in your use case, but I fear it was too specific a use case. It won't work in most instances because the element's font size won't be $base-font-size.

@scottdavis
Compass member

well thats fine then maybe this should just alias establish baseline?

@ry5n

@scottdavis I don't think it can alias +establish-baseline; that outputs font-size and line-height inside a body selector. I’ve never used this mixin myself but I understand the use case: somehow we have a nested element (like a section heading or entire ui component) whose line-height is 'off' from the vertical rhythm. We don't want to change the font-size of the element, just get its internal line-height re-aligned with the baseline grid as best we can. I think a rename to +match-base-line-height or +reset-line-height makes sense here. It would have to accept the context font size as an argument ($font-size, as do most rhythm mixins) and simply set line-height: rhythm(1, $font-size). [edit for typo]

@scottdavis
Compass member
@ry5n ry5n added a commit to ry5n/compass that referenced this pull request Mar 14, 2012
@ry5n ry5n Replace @mixin reset-baseline mixing.
Proposed correction for #780.
f304c6b
@ry5n ry5n added a commit to ry5n/compass that referenced this pull request Mar 14, 2012
@ry5n ry5n Replace +reset-baseline with +match-base-line-height
Proposed correction for #780 [with more sensible commit message]
3f57ca1
@ry5n

@JohnAlbin @scottdavis I've pushed a proposed change that replaces +reset-baseline with +match-base-line-height. However, having never needed the mixin myself despite heavy use of the module and knowing that it only aliases +adjust-leading-to(1, [$font-size]) I'm skeptical about the value of this change. I wonder if it may be better to simply deprecate the existing mixin.

(BTW sorry about the extra commit – that first commit message makes no sense).

@chriseppstein
Compass member

@ry5n I'm ok with deprecating this in 0.13 with no replacement. But 0.12 is released, so we can't make this change on stable.

Closing.

@scottdavis
Compass member

I guess im ok with a deprecation i can just make the helper mixin within my application

@scottdavis
Compass member

/passive agressive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment