Skip to content
This repository has been archived by the owner on Nov 21, 2019. It is now read-only.

Rewrite CSS margin and nospace mixins to use logical properties #13

Merged
merged 36 commits into from
Oct 3, 2018

Conversation

davidcmoulton
Copy link
Member

@davidcmoulton davidcmoulton commented Sep 26, 2018

  1. Extract guts of padding mixin into internal _spacing mixin.
  2. Rewrite _padding-left and _padding-right mixins to be _spacing-left and _spacing-right, respectively, in order to support both padding and margin.
  3. Refactor padding mixin to be a thin wrapper calling _spacing with the $space-type of padding.
  4. Write tests for margin mixin and implement using _spacing with the $space-type of margin.

Note: as we're not supporting vertical writing modes yet, block-start always sets top, and block-end always sets bottom.

@davidcmoulton davidcmoulton changed the title [WIP] Rewrite CSS margin and nospace mixins to use logical properties Rewrite CSS margin and nospace mixins to use logical properties Sep 27, 2018
}
}

@include it("Ignores more than two values in the sizes list for the dimension 'inline'") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me think it might be better to have more explicit mixins rather than having arguments that change what they do (eg @include margin-inline(16, 32)).

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the point, but not sure it's the right thing to do here: I'd rather have just one way of setting margins, at least to start off with. Using @stephenwf's idea of throwing errors when there are an incorrect number of values passed will supply rapid developer feedback, preventing the silent fail which this code currently allows.

$firstValue: nth($size_in_px, 1);
$secondValue: nth($size_in_px, 2);
$firstValue: nth($size-in-px, 1);
$secondValue: nth($size-in-px, 2);
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could @error here if there is a 3rd parameter?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no way of catching sass errors afaik, so we wouldn't be able to have a test that checked correct error-throwing behaviour. I think it's worth doing anyway though, good call.

@davidcmoulton
Copy link
Member Author

Doesn't yet work correctly when more than one value in the list for the first argument, when no dimension supplied.

@davidcmoulton
Copy link
Member Author

  • Works for multiple lengths without a dimension
  • Throws @errors at sass compilation time when an incorrect number of length values is supplied for the stated dimension. Tests that verified incorrect numbers of length values would be ignored have been removed as this situation now causes (desired) errors. (They can't be caught, so they can't be safely tested for.)

@davidcmoulton
Copy link
Member Author

Hmm... No way of setting just top or just bottom spacing at the moment, due to uncertainty of the vertical writing mode / block situation.

@davidcmoulton davidcmoulton changed the title Rewrite CSS margin and nospace mixins to use logical properties [WIP] Rewrite CSS margin and nospace mixins to use logical properties Sep 27, 2018
@thewilkybarkid
Copy link
Contributor

They can't be caught, so they can't be safely tested for.

Did you see oddbird/true#92 (comment)?

@davidcmoulton
Copy link
Member Author

@thewilkybarkid Hadn't seen that, cheers!

@davidcmoulton davidcmoulton changed the title [WIP] Rewrite CSS margin and nospace mixins to use logical properties Rewrite CSS margin and nospace mixins to use logical properties Sep 28, 2018
@davidcmoulton davidcmoulton merged commit 71700cf into master Oct 3, 2018
@davidcmoulton davidcmoulton deleted the css-margin-mixin branch October 3, 2018 09:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants