docs(rtl): Simplify README to follow best practices #917

lynnmercier · 2017-07-07T21:23:03Z

No description provided.

codecov-io · 2017-07-07T21:35:15Z

Codecov Report

Merging #917 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #917   +/-   ##
=======================================
  Coverage   99.93%   99.93%           
=======================================
  Files          68       68           
  Lines        3276     3276           
  Branches      403      403           
=======================================
  Hits         3274     3274           
  Misses          2        2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b001aec...5c52971. Read the comment docs.

yeelan0319 · 2017-07-17T16:12:04Z

packages/mdc-rtl/README.md

+| Mixin | Description |
+| ----------------------------------------------- | - |
+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |
+| `mdc-rtl-reflexive-box($base-property, $default-direction, $value, $root-selector)` | Applies the value to the `#{$base-property}-#{$default-direction}` property  in a LTR context, and flips the direction in an RTL context. This mixin zeros out the original value in an RTL context. |


find a typo in comments here in mdc-rtl/mixin.scss line 128:

* You can also pass a 4th optional $root-selector argument which will be forwarded to `mdc-rtl`, * e.g. `@include mdc-rtl-reflexive-box-property(margin, left, 8px, ".mdc-component")`.

It should be @include mdc-rtl-reflexive-box, we don't have a mixin called mdc-rtl-reflexive-box-property

yeelan0319 · 2017-07-17T16:18:34Z

packages/mdc-rtl/README.md

+| Mixin | Description |
+| ----------------------------------------------- | - |
+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |
+| `mdc-rtl-reflexive-box($base-property, $default-direction, $value, $root-selector)` | Applies the value to the `#{$base-property}-#{$default-direction}` property  in a LTR context, and flips the direction in an RTL context. This mixin zeros out the original value in an RTL context. |


I suggest we emphasize the size effect this mixin have, it zeros out the counter-party value, which might surprise our user in some case.

Try to emphasize this point :) WDYT?

yeelan0319 · 2017-07-17T16:19:30Z

packages/mdc-rtl/README.md

+| ----------------------------------------------- | - |
+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |
+| `mdc-rtl-reflexive-box($base-property, $default-direction, $value, $root-selector)` | Applies the value to the `#{$base-property}-#{$default-direction}` property  in a LTR context, and flips the direction in an RTL context. This mixin zeros out the original value in an RTL context. |
+| `mdc-rtl-reflexive-property($base-property, $left-value, $right-value, $root-selector)` | Emits rules that assign `#{$base-property}`-left to `#{left-value}` and `#{base-property}`-right to `#{right-value}` in a LTR context, and vice versa in a RTL context. |


Put a line when we should use mdc-rtl-reflexive-property instead of mdc-rtl-reflexive-box by a quick comparison between its difference?

yeelan0319 · 2017-07-17T16:25:18Z

packages/mdc-rtl/README.md

+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |
+| `mdc-rtl-reflexive-box($base-property, $default-direction, $value, $root-selector)` | Applies the value to the `#{$base-property}-#{$default-direction}` property  in a LTR context, and flips the direction in an RTL context. This mixin zeros out the original value in an RTL context. |
+| `mdc-rtl-reflexive-property($base-property, $left-value, $right-value, $root-selector)` | Emits rules that assign `#{$base-property}`-left to `#{left-value}` and `#{base-property}`-right to `#{right-value}` in a LTR context, and vice versa in a RTL context. |
+| `mdc-rtl-reflexive-position($pos, $value, $root-selector)` | Applies the value to the specified position in a LTR context, and flips the direction in an RTL context. |



IMHO, having a short example will help me understand the behavior of the mixin classes better than have only text. If you feel it is too long, I suggest we change the verbose example to a simpler one.

I want developers to read about what each mixin accomplishes. This lets developers decide where they can use the mixins in their own code. Once a developer is using a mixin, we can upgrade the implementation of the mixin so long as it still accomplishes the same original goal.

When we document code examples of the actual CSS output of the mixin, we risk developers using a mixin simply because they want that exact output...NOT because they want to accomplish the same goal the mixin aims to solve.

So we can document examples of where to use this mixins, but we should avoid documenting the actual CSS output. The CSS output is an implementation detail.

...I looked at our code and I couldn't actually find great examples of us using these properties...Theres one in checkbox, but it's more confusing to me than helpful.

Pointed well taken, I agree that we shouldn't include transpiled css here. However, the single line document confuse me when to use which mixin class. Could we consider having a paragraph like:

In case for symmetric RTL properties, you can use mdc-rtl-reflexive-box. For example, when button have margin-left: 8px; margin-right: 0px; to left edge and margin-left: 0px; margin-right: 8px to right edge in RTL mode.

@include mdc-rtl-reflexive-box('margin', 'left', 8px, '.mdc-foo')

yeelan0319 · 2017-07-17T16:28:07Z

packages/mdc-rtl/README.md


-### mdc-rtl
+`$base-property` is a base box-model property. e.g. margin / border / padding.


Listing the args out here looks a bit unnatural to me. Can we put it as part of the following table and use a more explanatory name, such as position-property?

lynnmercier · 2017-07-18T17:41:15Z

Addressed comments, PTAL

yeelan0319 · 2017-07-18T20:27:37Z

packages/mdc-rtl/README.md

+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |
+| `mdc-rtl-reflexive-box($base-property, $default-direction, $value, $root-selector)` | Applies the value to the `#{$base-property}-#{$default-direction}` property  in a LTR context, and flips the direction in an RTL context. This mixin zeros out the original value in an RTL context. |
+| `mdc-rtl-reflexive-property($base-property, $left-value, $right-value, $root-selector)` | Emits rules that assign `#{$base-property}`-left to `#{left-value}` and `#{base-property}`-right to `#{right-value}` in a LTR context, and vice versa in a RTL context. |
+| `mdc-rtl-reflexive-position($pos, $value, $root-selector)` | Applies the value to the specified position in a LTR context, and flips the direction in an RTL context. |



Pointed well taken, I agree that we shouldn't include transpiled css here. However, the single line document confuse me when to use which mixin class. Could we consider having a paragraph like:

In case for symmetric RTL properties, you can use mdc-rtl-reflexive-box. For example, when button have margin-left: 8px; margin-right: 0px; to left edge and margin-left: 0px; margin-right: 8px to right edge in RTL mode.

@include mdc-rtl-reflexive-box('margin', 'left', 8px, '.mdc-foo')

yeelan0319 · 2017-07-18T20:28:15Z

packages/mdc-rtl/README.md

+| Mixin | Description |
+| ----------------------------------------------- | - |
+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |
+| `mdc-rtl-reflexive-box($base-property, $default-direction, $value, $root-selector)` | Applies the value to the `#{$base-property}-#{$default-direction}` property  in a LTR context, and flips the direction in an RTL context. This mixin zeros out the original value in an RTL context. |


Try to emphasize this point :) WDYT?

yeelan0319 · 2017-07-18T20:28:44Z

packages/mdc-rtl/README.md


-### mdc-rtl
+`$base-property` is a base box-model property. e.g. margin / border / padding.
+`$pos` is a horizontal position property, either "left" or "right".


We should also update here

Sorry...but I think maybe you're look at an old commit. I changed this in the latest commit.

Or maybe I screwed something up in my commits...

yeelan0319 · 2017-07-18T20:29:00Z

packages/mdc-rtl/README.md

+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |
+| `mdc-rtl-reflexive-box($base-property, $default-direction, $value, $root-selector)` | Applies the value to the `#{$base-property}-#{$default-direction}` property  in a LTR context, and flips the direction in an RTL context. This mixin zeros out the original value in an RTL context. |
+| `mdc-rtl-reflexive-property($base-property, $left-value, $right-value, $root-selector)` | Emits rules that assign `#{$base-property}`-left to `#{left-value}` and `#{base-property}`-right to `#{right-value}` in a LTR context, and vice versa in a RTL context. |
+| `mdc-rtl-reflexive-position($pos, $value, $root-selector)` | Applies the value to the specified position in a LTR context, and flips the direction in an RTL context. |


lynnmercier · 2017-07-18T22:08:52Z

Added another paragraph about how each RTL mixin kind of "builds" on the previous one.

yeelan0319 · 2017-07-20T04:43:13Z

packages/mdc-rtl/README.md

-```
+| Mixin | Description |
+| ----------------------------------------------- | - |
+| `mdc-rtl($root-selector)` | Creates a rule that is applied when the root element is within an RTL context |


nit: Create rules applied within $root-selector scope when the root element is contained within an RTL context

yeelan0319 · 2017-07-20T04:45:23Z

packages/mdc-rtl/README.md


-### mdc-rtl
+`mdc-rtl` is the most flexible mixin, because it can work with multiple CSS properties. All other RTL mixins logic could be engineered by only using `mdc-rtl`, but we provide these mixins for convenience.


nit and opnionated:

but we recommend using a more specific mixin below for better code readability

yeelan0319 · 2017-07-20T04:46:20Z

packages/mdc-rtl/README.md

-    left: auto;
-    right: 0;
-  }
+`mdc-rtl-reflexive-position` is the least flexible mixin. It only works with one horizontal position property, "left" or "right". It also assumes the left and right values are the same.


less flexible -> most specific mixin about position?

docs(rtl): Simplify README to follow best practices

da04472

yeelan0319 suggested changes Jul 17, 2017

View reviewed changes

lynnmercier added 2 commits July 18, 2017 10:20

improve README and some fixes to _mixins.scss

0ac8ba7

Merge branch 'master' into docs/rtl

28afaf2

yeelan0319 suggested changes Jul 18, 2017

View reviewed changes

add paragraph about how each rtl mixin builds on the previous

b41e56e

Merge branch 'master' into docs/rtl

1c599be

yeelan0319 approved these changes Jul 20, 2017

View reviewed changes

lynnmercier added 2 commits July 20, 2017 09:47

Merge branch 'master' into docs/rtl

0571a94

Merge branch 'master' into docs/rtl

5c52971

lynnmercier merged commit e145e37 into master Jul 20, 2017

lynnmercier deleted the docs/rtl branch July 26, 2017 20:54


		### mdc-rtl
		`$base-property` is a base box-model property. e.g. margin / border / padding.


		### mdc-rtl
		`mdc-rtl` is the most flexible mixin, because it can work with multiple CSS properties. All other RTL mixins logic could be engineered by only using `mdc-rtl`, but we provide these mixins for convenience.

docs(rtl): Simplify README to follow best practices #917

docs(rtl): Simplify README to follow best practices #917

Uh oh!

Conversation

lynnmercier commented Jul 7, 2017

Uh oh!

codecov-io commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynnmercier commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lynnmercier commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Jul 7, 2017 •

edited

Loading