Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FeatureHighlight] FeatureHighlightViewController should expose FeatureHighlightView as a property. #3145

Conversation

mohammadcazig
Copy link
Contributor

Expose FeatureHighlightView property from FeatureHighlightViewController.
Removing Redundant properties from FeatureHighlightViewController
Internal Clean up to simplify dynamic font logic.
Updating the tests.
Close #3144

@ianegordon ianegordon added the PR is blocked Please explain why. label Mar 26, 2018
@ianegordon
Copy link
Contributor

Blocked until we determine our theming strategy.

@@ -250,51 +219,35 @@ - (void)setTitleColor:(UIColor *)titleColor {
_titleLabel.textColor = titleColor;
}

- (UIFont *)defaultTitleFont {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make into a class method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a readonly class property instead of a method? Do we have a preference either way?

Copy link
Contributor

Choose a reason for hiding this comment

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

Property FTW.

[self updateBodyFont];
}

- (UIFont *)defaultBodyFont {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make into a class method.

@ianegordon
Copy link
Contributor

Kokoro failure is unrelated.

@ianegordon ianegordon merged commit 24be789 into material-components:develop Mar 27, 2018

Alpha should be set to kMDCFeatureHighlightOuterHighlightAlpha.
*/
@property(nonatomic, strong, null_resettable) UIColor *outerHighlightColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

there are internal clients that are using this and the other properties. We should mark them deprecated instead of going strait to breaking changes.

randallli added a commit that referenced this pull request Mar 30, 2018
…se FeatureHighlightView as a property. (#3145)"

This reverts commit 24be789.
@romoore
Copy link
Contributor

romoore commented Apr 4, 2018

This can result in slightly odd-looking code. Is this how we expect clients to use the new API?

// Set the content on the VC
highlightController.titleText = @"Look, a New Feature!";
highlightController.bodyText = @"Look at this great new feature. It's great!";

// Set the colors on the view
highlightController.featureHighlightView.titleColor = UIColor.whiteColor;
highlightController.featureHighlightView.bodyColor = UIColor.whiteColor;
highlightController.featureHighlightView.outerHighlightColor = UIColor.darkGrayColor;

romoore pushed a commit that referenced this pull request Apr 4, 2018
…se FeatureHighlightView as a property. (#3145)"

Many internal clients are breaking because of this API change. Instead, we
should follow the deprecation policy so clients can make a gradual migration
to the final API.

This reverts commit 24be789.

Reopens #3144
romoore pushed a commit that referenced this pull request Apr 4, 2018
…se FeatureHighlightView as a property. (#3145)" (#3231)

Many internal clients are breaking because of this API change. Instead, we
should follow the deprecation policy so clients can make a gradual migration
to the final API.

This reverts commit 24be789.

Reopens #3144
@mohammadcazig mohammadcazig deleted the Feature_Highlights_Expose_Typography_API branch April 6, 2018 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes PR is blocked Please explain why.
Projects
None yet
5 participants