Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(): refactor rtl-prop mixin to add less CSS #9218

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Aug 3, 2016

  • Refactors the rtl-prop mixin to include less CSS.
  • Changes all the instances where it was used to incorporate the new syntax.

Note: There are a few instances where the mixin seems used redundantly. These aren't the objective of this PR.

Fixes #9217.

* Refactors the rtl-prop mixin to include less CSS.
* Changes all the instances where it was used to incorporate the new syntax.

**Note:** There are a few instances where the mixin seems used redundantly. These aren't the objective of this PR.

Fixes angular#9217.
@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label Aug 3, 2016
@@ -125,15 +125,10 @@
}
}

@mixin rtl-prop($ltr-prop, $rtl-prop, $value) {
@mixin rtl-prop($ltr-prop, $rtl-prop, $value, $reset-value) {
Copy link
Contributor

@clshortfuse clshortfuse Aug 3, 2016

Choose a reason for hiding this comment

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

I would say you should add a null default:
@mixin rtl-prop($ltr-prop, $rtl-prop, $value, $reset-value: null) {

That was, we can make some segments with even less CSS.

http://www.sassmeister.com/gist/b87f3d64603d5bceaa29534ffc585457

Edit: Actually, on second inspection, can't think of a use for not specifying a reset value.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point of the PR isn't to reduce the CSS as much as possible, but rather to remove the redundancies while staying as close to the old functionality as possible.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: review This PR is waiting on review from the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants