[Buttons] Implement a color themer for an MDCFlatButton #3308

yarneo · 2018-04-10T13:08:08Z

Addition of a unit test, and updated example to use the new themer.

Pivotal: https://www.pivotaltracker.com/story/show/156640819

…ents-ios into flatButtonTheming

romoore · 2018-04-10T13:10:28Z

components/Buttons/src/ColorThemer/MDCButtonColorThemer.h

@@ -30,6 +30,14 @@
 + (void)applySemanticColorScheme:(nonnull id<MDCColorScheming>)colorScheme
                        toButton:(nonnull MDCButton *)button;

+/**
+ Applies a color scheme to theme to a MDCFlatButton.


Super-nit: Should be "an MDCFlatButton". Please don't fix unless you have other changes.

updated and also other "a" -> "an" in the rest of the header for consistency.

romoore · 2018-04-10T13:12:05Z

components/Buttons/src/ColorThemer/MDCButtonColorThemer.h

+/**
+ Applies a color scheme to theme to a MDCFlatButton.
+
+ @param colorScheme The color scheme to apply to MDCFlatButton.


Nit: Should be, "... to apply to @c flatButton." (referencing the other parameter).

updated, and rest of header to comply with this.

romoore · 2018-04-10T13:12:25Z

components/Buttons/src/ColorThemer/MDCButtonColorThemer.h

+ Applies a color scheme to theme to a MDCFlatButton.
+
+ @param colorScheme The color scheme to apply to MDCFlatButton.
+ */


LINT: Missing param for flatButton

thanks, updated

romoore · 2018-04-10T13:13:12Z

components/Buttons/src/ColorThemer/MDCButtonColorThemer.m

+  [flatButton setBackgroundColor:[colorScheme.primaryColor colorWithAlphaComponent:0.f]
+                        forState:UIControlStateHighlighted];
+  [flatButton setBackgroundColor:[colorScheme.primaryColor colorWithAlphaComponent:0.f]
+                        forState:UIControlStateSelected];


Just curious, but why use "primaryColor" with alpha 0 instead of UIColor.clearColor or nil? Also, can you not depend on the "fallback" behavior for the background color and only set the UIControlStateNormal value? I'm curious about the result if another class sets a combination of the states (since they are an NSOption), like UIControlStateSelected | UIControlStateHighlighted.

+1 that these should likely just be clear.

Also, can you not depend on the "fallback" behavior for the background color and only set the UIControlStateNormal value?

There's some nuance here. I think the expectation of our themers is that they enforce a particular theme on a component. If someone else had modified any of these states prior to us applying the theme then presumably we'd want to blow away those modifications. This unfortunately may mean we have to iterate over every state combination...but I'm really unhappy about that as a solution as well. What do y'all think?

I prefer to rely on fallback behavior - I think most clients will expect that. UIKit tends to document that same expectation. It's often worded like, "at a minimum set the title for the .normal state." If we are going to pick out specific states (like .disabled), there should be a difference between that state and .normal.

I had the same approach in terms of the background color, but I wanted to follow the params doc guidelines and there they stated primaryColor + opacity of 0.

Yeah, the reason I set all the states is because if a client changed the color of a state prior to the theming, the theming wont override that color to its rightful theming.

The params doc unfortunately doesn't have any awareness of a "clear" color, so they specify colors in terms of color + opacity. This isn't a color that is intended to be mapped by the color system because the opacity is always going to be 0, so for our purposes I believe it's sufficient to use clearColor.

sgtm, will update accordingly

So should all of the states be added to the map instead of nilling-out and relying on the fallback? I'm curious because this affects how I write a lot of component/themer code in general.

after speaking to both, have moved to a fallback to .normal approach with .normal and .disabled set in the themer.

romoore · 2018-04-10T13:15:08Z

components/Buttons/tests/unit/ButtonsColorThemerTests.m

+  XCTAssert(
+      CGColorEqualToColor([button backgroundColorForState:UIControlStateDisabled].CGColor,
+                          [colorScheme.primaryColor colorWithAlphaComponent:0.f].CGColor));
+  XCTAssertEqual(button.disabledAlpha, 1.f);


Please use XCTAssertEqualWithAccuracy for floating point comparison.

ianegordon · 2018-04-10T15:15:35Z

components/Buttons/examples/ButtonsTypicalUse.m

@@ -43,6 +43,8 @@ - (MDCButton *)buildCustomStrokedButton {
 - (void)viewDidLoad {
  [super viewDidLoad];

+  id<MDCColorScheming> colorScheme = [[MDCSemanticColorScheme alloc] init];


I typically expect protocols in the context of interfaces, methods and delegate.
It seems strange to have it as the sole reference to a concrete object.
Consider changing to:
MDCSemanticColorScheme *colorScheme = [[MDCSemanticColorScheme alloc] init];

jverkoey · 2018-04-10T16:50:42Z

components/Buttons/src/ColorThemer/MDCButtonColorThemer.m

+  [flatButton setTitleColor:colorScheme.primaryColor forState:UIControlStateNormal];
+  [flatButton setTitleColor:[colorScheme.onSurfaceColor colorWithAlphaComponent:0.26f]
+                   forState:UIControlStateDisabled];
+  flatButton.disabledAlpha = 1.f;


@romoore @yarneo Should we be resetting all of the states to nil before setting our own colors here?

I would suggest two parts:

Set only those states set by the target class. Nilling-out anything that should fall-back to .normal

Write unit tests to verify the behavior of all possible states.

updated :) PTAL

yarneo and others added 30 commits April 10, 2018 13:25

added color themer for MDCButton

6e53ade

Update ButtonsColorThemerTests.m

07946ea

Update ButtonsColorThemerTests.m

9442abd

Update ButtonsColorThemerTests.m

40d1ffe

Update ButtonsColorThemerTests.m

7732d37

Update ButtonsColorThemerTests.m

10098e1

initial imp

c4a4a57

Merge branch 'buttonColorThemer' of github.com:yarneo/material-compon…

21aa78d

…ents-ios into flatButtonTheming

Update MDCButtonColorThemer.h

ae6b5d0

update

b23520d

updated disabledAlpha

941167c

remove disabled alpha

89a1ec5

initial imp

1f4f4af

update

c35138c

update

e19c2e9

Merge branch 'flatButtonTheming' of github.com:yarneo/material-compon…

bea202b

…ents-ios into flatButtonTheming

added color themer for MDCButton

9bd6657

Update ButtonsColorThemerTests.m

f0212c4

Update ButtonsColorThemerTests.m

eba22ad

Update ButtonsColorThemerTests.m

1036185

Update ButtonsColorThemerTests.m

b7bfd14

Update MDCButtonColorThemer.h

5a9d849

updated disabledAlpha

101cbed

remove disabled alpha

1553bca

initial imp

0d3437d

update

d7c0e63

update

4406c55

initial imp

1d6a6c0

update

098969d

remove duplicate test

4c8a9af

remove duplicate test

db72255

yarneo requested a review from a team April 10, 2018 13:08

googlebot added the cla: yes label Apr 10, 2018

yarneo added the [Buttons] label Apr 10, 2018

romoore suggested changes Apr 10, 2018

View reviewed changes

wording and accuracyXCTesting

7b16edc

ianegordon reviewed Apr 10, 2018

View reviewed changes

yarneo added 2 commits April 10, 2018 18:46

update based on review

236f77a

update based on review

02043d6

jverkoey reviewed Apr 10, 2018

View reviewed changes

update per review

3f64e25

jverkoey approved these changes Apr 11, 2018

View reviewed changes

Merge branch 'develop' into flatButtonTheming

75499ed

yarneo merged commit 482bd56 into material-components:develop Apr 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Buttons] Implement a color themer for an MDCFlatButton #3308

[Buttons] Implement a color themer for an MDCFlatButton #3308

yarneo commented Apr 10, 2018

romoore Apr 10, 2018

yarneo Apr 10, 2018

romoore Apr 10, 2018

yarneo Apr 10, 2018

romoore Apr 10, 2018

yarneo Apr 10, 2018

romoore Apr 10, 2018

jverkoey Apr 10, 2018

jverkoey Apr 10, 2018

romoore Apr 10, 2018

yarneo Apr 10, 2018

yarneo Apr 10, 2018

jverkoey Apr 10, 2018

yarneo Apr 10, 2018

romoore Apr 10, 2018 •

edited

yarneo Apr 10, 2018

romoore Apr 10, 2018

yarneo Apr 10, 2018

ianegordon Apr 10, 2018

yarneo Apr 10, 2018

jverkoey Apr 10, 2018

romoore Apr 10, 2018

yarneo Apr 11, 2018

[Buttons] Implement a color themer for an MDCFlatButton #3308

[Buttons] Implement a color themer for an MDCFlatButton #3308

Conversation

yarneo commented Apr 10, 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

romoore Apr 10, 2018 • edited

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

romoore Apr 10, 2018 •

edited