[Shapes] Initial Shape Scheme implementation #5014

yarneo · 2018-09-05T18:30:58Z

This PR implements the initial Shape Scheme that is essential for allowing shape theming for components (this doesn't include any themers and that will be included as a separate PR once this is approved). More information can be seen in go/mdc-ios-shape-theming and go/material-shapes-eng

This closes #4609 #4612 #4613

material-automation · 2018-09-05T18:31:01Z

This PR affects multiple components.

material-automation · 2018-09-05T18:32:58Z

components/schemes/Shape/src/MDCShapeCorner.h

+  MDCShapeCornerSizeTypePercentage,
+};
+
+


clang-format suggested the following change:

-

material-automation · 2018-09-05T18:32:59Z

components/schemes/Shape/src/MDCShapeCorner.h

+ @return an MDCShapeCorner.
+ */
+- (instancetype)initWithFamily:(MDCShapeCornerFamily)cornerFamily
+                       andSize:(CGFloat)cornerSize;


clang-format suggested the following change:

-- (instancetype)initWithFamily:(MDCShapeCornerFamily)cornerFamily - andSize:(CGFloat)cornerSize; +- (instancetype)initWithFamily:(MDCShapeCornerFamily)cornerFamily andSize:(CGFloat)cornerSize;

material-automation · 2018-09-05T18:33:01Z

components/schemes/Shape/src/MDCShapeCategory.h

+ */
+@property(strong, nonatomic) MDCShapeCorner *bottomRightCorner;
+
+


clang-format suggested the following change:

-

material-automation · 2018-09-05T18:33:03Z

components/private/ShapeLibrary/src/MDCCornerTreatment+CornerTypeInitalizer.m

+        CGFloat radiusSize = [size floatValue];
+        self = [[MDCRoundedCornerTreatment alloc] initWithRadius:radiusSize];
+        break;
+      }


clang-format suggested the following change:

- self = [self init]; - switch(cornerType) { - case MDCCornerTypeCurved: { - CGSize curvedSize = [size CGSizeValue]; - self = [[MDCCurvedCornerTreatment alloc] initWithSize:curvedSize]; - break; - } - case MDCCornerTypeCut: { - CGFloat cutSize = [size floatValue]; - self = [[MDCCutCornerTreatment alloc] initWithCut:cutSize]; - break; - } - case MDCCornerTypeRounded: { - CGFloat radiusSize = [size floatValue]; - self = [[MDCRoundedCornerTreatment alloc] initWithRadius:radiusSize]; - break; - } + self = [self init]; + switch (cornerType) { + case MDCCornerTypeCurved: { + CGSize curvedSize = [size CGSizeValue]; + self = [[MDCCurvedCornerTreatment alloc] initWithSize:curvedSize]; + break;

material-automation · 2018-09-05T18:33:05Z

components/private/ShapeLibrary/src/MDCCornerTreatment+CornerTypeInitalizer.m

+        CGFloat radiusSize = [size floatValue];
+        self = [[MDCRoundedCornerTreatment alloc] initWithRadius:radiusSize];
+        break;
+      }


clang-format suggested the following change:

+ case MDCCornerTypeCut: { + CGFloat cutSize = [size floatValue]; + self = [[MDCCutCornerTreatment alloc] initWithCut:cutSize]; + break; + } + case MDCCornerTypeRounded: { + CGFloat radiusSize = [size floatValue]; + self = [[MDCRoundedCornerTreatment alloc] initWithRadius:radiusSize]; + break; + } + }

material-automation · 2018-09-05T18:33:07Z

components/private/ShapeLibrary/src/MaterialShapeLibrary.h

@@ -19,3 +19,4 @@
 #import "MDCSlantedRectShapeGenerator.h"
 #import "MDCTriangleEdgeTreatment.h"
 #import "MDCCutCornerTreatment.h"
+#import "MDCCornerTreatment+CornerTypeInitalizer.h"


clang-format suggested the following change:

-#import "MDCCutCornerTreatment.h" -#import "MDCCornerTreatment+CornerTypeInitalizer.h"

material-automation · 2018-09-05T18:33:09Z

components/schemes/Shape/src/MDCShapeCategory.m

+@implementation MDCShapeCategory
+
+- (instancetype)init
+{


clang-format suggested the following change:

-- (instancetype)init -{ +- (instancetype)init {

material-automation · 2018-09-05T18:33:10Z

components/schemes/Shape/src/MDCShapeCategory.m

+}
+
+- (instancetype)initCornersWithFamily:(MDCShapeCornerFamily)cornerFamily
+                             andSize:(CGFloat)cornerSize {


clang-format suggested the following change:

- andSize:(CGFloat)cornerSize { + andSize:(CGFloat)cornerSize {

material-automation · 2018-09-05T18:33:12Z

components/schemes/Shape/src/MDCShapeCategory.m

+    _bottomLeftCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily
+                                                       andSize:cornerSize];
+    _bottomRightCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily
+                                                        andSize:cornerSize];


clang-format suggested the following change:

- _topLeftCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily - andSize:cornerSize]; - _topRightCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily - andSize:cornerSize]; - _bottomLeftCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily - andSize:cornerSize]; - _bottomRightCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily - andSize:cornerSize]; + _topLeftCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily andSize:cornerSize]; + _topRightCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily andSize:cornerSize]; + _bottomLeftCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily andSize:cornerSize]; + _bottomRightCorner = [[MDCShapeCorner alloc] initWithFamily:cornerFamily andSize:cornerSize];

material-automation · 2018-09-05T18:33:14Z

components/schemes/Shape/src/MDCShapeScheme.m

+}
+
+@end
+


clang-format suggested the following change:

-

material-automation · 2018-09-05T18:33:15Z

components/schemes/Shape/src/MDCShapeCorner.m

+@implementation MDCShapeCorner
+
+- (instancetype)init
+{


clang-format suggested the following change:

-- (instancetype)init -{ +- (instancetype)init {

material-automation · 2018-09-05T18:33:17Z

components/schemes/Shape/src/MDCShapeCorner.m

+}
+
+- (instancetype)initWithFamily:(MDCShapeCornerFamily)cornerFamily
+                       andSize:(CGFloat)cornerSize {


clang-format suggested the following change:

-- (instancetype)initWithFamily:(MDCShapeCornerFamily)cornerFamily - andSize:(CGFloat)cornerSize { +- (instancetype)initWithFamily:(MDCShapeCornerFamily)cornerFamily andSize:(CGFloat)cornerSize {

material-automation · 2018-09-05T18:33:19Z

components/schemes/Shape/src/MDCShapeCorner.m

+    case MDCShapeCornerFamilyAngled:
+      cornerTreatment =
+          [[MDCCornerTreatment alloc] initWithCornerType:MDCCornerTypeCut
+                                                 andSize:size];


clang-format suggested the following change:

- cornerTreatment = - [[MDCCornerTreatment alloc] initWithCornerType:MDCCornerTypeCut - andSize:size]; + cornerTreatment = [[MDCCornerTreatment alloc] initWithCornerType:MDCCornerTypeCut + andSize:size];

material-automation · 2018-09-05T18:33:20Z

components/schemes/Shape/src/MDCShapeCorner.m

+    case MDCShapeCornerFamilyRounded:
+      cornerTreatment =
+          [[MDCCornerTreatment alloc] initWithCornerType:MDCCornerTypeRounded
+                                                 andSize:size];


clang-format suggested the following change:

- cornerTreatment = - [[MDCCornerTreatment alloc] initWithCornerType:MDCCornerTypeRounded - andSize:size]; + cornerTreatment = [[MDCCornerTreatment alloc] initWithCornerType:MDCCornerTypeRounded + andSize:size];

material-automation · 2018-09-05T18:33:24Z

API diff detected the following changes

ShapeScheme

New component.

material-automation · 2018-09-05T18:39:27Z

This PR affects multiple components.

jverkoey · 2018-09-05T18:42:24Z

components/schemes/Shape/src/MaterialShapeScheme.h

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#import "MDCShapeScheme.h"


Add #import "MDCShapeCategory.h" as well.

For that matter, all of the public header should be listed here.

material-automation · 2018-09-05T18:48:12Z

This PR affects multiple components.

jverkoey

LGTM with mostly documentation feedback.

jverkoey · 2018-09-05T18:43:46Z

components/schemes/Shape/src/MDCShapeCategory.h

+
+ MDCShapeCategory is built from 4 corners, that can be set to alter the shape value.
+
+ This takes on the assumption that our settable shapes are made of 4 corners.


I'm not sure this sentence is communicating any new information (I think it's describing the fact that the class has four corners) - consider removing?

components/schemes/Shape/src/MDCShapeCategory.h

components/schemes/Shape/src/MDCShapeCorner.h

jverkoey · 2018-09-05T18:49:28Z

components/schemes/Shape/src/MDCShapeCorner.h

+ that are positive.
+
+ When MDCShapeSizeType is MDCShapeCornerSizeTypePercentage, this accepts percentage values
+ from 0 to 1 (0% - 100%). These values are percentages based on the height of the surface.


The wording here implies that values outside of these ranges might thrown an assertion. If this is the case, document that the bounds are strictly enforced. Otherwise it may help to soften the wording here to say "values are expected to be in the range of [lower...upper]"

jverkoey · 2018-09-05T18:50:21Z

components/schemes/Shape/src/MDCShapeCorner.h

+
+ @return an MDCCornerTreatment.
+ */
+- (MDCCornerTreatment *)cornerTreatmentValue;


Why do we have this layer of indirection? Do we intend to deprecate or replace MDCCornerTreatment?

Put another way - why is the logic that turns a shape corner into a corner treatment not part of the themers instead?

I tried to make it so we only have to do the conversion of MDCShapeCorner to one of the MDCCornerTreatment instances once, rather than it being done in every one of our themers. I initially had it in the themers but it was rather messy and redundant. In terms of architecture it made sense to add a convenience converter in MDCShapeCorner and then add additional initializers to MDCCornerTreatment to take the needed information and initialize the right corner treatment. Do you think there is a better way?

Do we intend to deprecate or replace MDCCornerTreatment?

What are your thoughts on this question?

I think MDCCornerTreatment fits the shape subsystem's purpose of setting corner treatments inside an MDCRectangleShapeGenerator well. I also think that the MDCShapeCorner encapsulates the exact relevant information needed to identify the corner values. The convertor is necessary as these two classes don't serve the same purpose. I do think that MDCCornerTreatment is using outdated terminology, i.e., CutCorner doesn't exist anymore, it is AngledCorner.

I see. I'm not entirely convinced yet that there needs to be two APIs that accomplish similar things with different terminology and naming. I see that you feel the two APIs accomplish very specific and different purposes, but this makes me wonder if we got the API wrong in the Shapes library and should update it accordingly. The end result being that MDCShapeCorner is part of the shapes library as a replacement for MDCCornerTreatment and the scheme relies on that accordingly.

If the above is sensible I think it's fair to plan a deprecation roadmap for the two APIs that will happen over a longer period of time, but that that work wouldn't necessarily have to block this PR.

If we think we want this to be one entity, we can always add these properties to MDCCornerTreatment and the right initializers to it as well. I felt like the division kept it cleaner. If you feel strongly about it, i can do it in this PR, as this isn't a deprecation per-say.

…nents-ios into initialshapescheme

material-automation · 2018-09-05T19:39:27Z

This PR affects multiple components.

material-automation · 2018-09-05T19:43:15Z

This PR affects multiple components.

jverkoey · 2018-09-05T19:58:00Z

components/schemes/Shape/src/MDCShapeCorner.m

+
+- (MDCCornerTreatment *)cornerTreatmentValueWithViewBounds:(CGRect)bounds {
+  MDCCornerTreatment *cornerTreatment;
+  if (_sizeType == MDCShapeCornerSizeTypePercentage && _size <= 1) {


This behavior looks a bit surprising if the size > 1 and we fall back to absolute size. Consider throwing an assert instead in order to enforce proper usage of the API.

true, i removed the check as I don't want to enforce it.

material-automation · 2018-09-05T20:12:22Z

This PR affects multiple components.

material-automation · 2018-09-05T20:13:52Z

This PR affects multiple components.

material-automation · 2018-09-06T00:45:06Z

This PR affects multiple components.

yarneo · 2018-09-06T15:01:44Z

This will be merged in the afternoon.

yarneo added 2 commits September 4, 2018 17:20

initial commit

5e091a3

update

afdae8f

yarneo requested review from jverkoey and a team September 5, 2018 18:30

material-automation bot added [private] [schemes] labels Sep 5, 2018

googlebot added the cla: yes label Sep 5, 2018

material-automation reviewed Sep 5, 2018

View reviewed changes

components/schemes/Shape/src/MDCShapeScheme.m Outdated

}

@end

Copy link

material-automation Sep 5, 2018

Choose a reason for hiding this comment

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

clang-format suggested the following change:

-

material-automation reviewed Sep 5, 2018

View reviewed changes

update clang

fbd4acb

material-automation bot added [private] [schemes] labels Sep 5, 2018

jverkoey reviewed Sep 5, 2018

View reviewed changes

Update MDCShapeCorner.h

d03e417

material-automation bot added [private] [schemes] labels Sep 5, 2018

jverkoey approved these changes Sep 5, 2018

View reviewed changes

yarneo added 2 commits September 5, 2018 15:38

update per review

00b055c

Merge branch 'initialshapescheme' of github.com:yarneo/material-compo…

3cadb46

…nents-ios into initialshapescheme

material-automation bot added [private] [schemes] labels Sep 5, 2018

update

ac68f3e

material-automation bot added [private] [schemes] labels Sep 5, 2018

jverkoey reviewed Sep 5, 2018

View reviewed changes

remove check

05b4902

material-automation bot added [private] [schemes] labels Sep 5, 2018

update podspec

03429fa

material-automation bot added [private] [schemes] labels Sep 5, 2018

update podspec

cad441b

material-automation bot added [private] [schemes] labels Sep 6, 2018

yarneo merged commit 97b830a into material-components:develop Sep 6, 2018

		*/
		@property(strong, nonatomic) MDCShapeCorner *bottomRightCorner;


		MDCShapeCategory is built from 4 corners, that can be set to alter the shape value.

		This takes on the assumption that our settable shapes are made of 4 corners.

[Shapes] Initial Shape Scheme implementation #5014

[Shapes] Initial Shape Scheme implementation #5014

Conversation

yarneo commented Sep 5, 2018

material-automation bot commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

material-automation commented Sep 5, 2018

API diff detected the following changes

ShapeScheme

material-automation bot commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

material-automation bot commented Sep 5, 2018

jverkoey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

material-automation bot commented Sep 5, 2018

material-automation bot commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

material-automation bot commented Sep 5, 2018

material-automation bot commented Sep 5, 2018

material-automation bot commented Sep 6, 2018

yarneo commented Sep 6, 2018