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.

Sign up for GitHub

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

Jump to bottom

Add support for tintColor on ASImageNode and ASButtonNode #1603

Merged
merged 8 commits into from
Aug 12, 2019
Merged

Add support for tintColor on ASImageNode and ASButtonNode #1603

merged 8 commits into from
Aug 12, 2019

Conversation

rahul-malik
Copy link
Contributor

Adding the ability to specify a tintColor for images / buttons that behave similarly to how they would under UIKit

@rahul-malik rahul-malik requested review from maicki and nguyenhuy July 30, 2019 23:57
bolsinga
bolsinga suggested changes Jul 31, 2019
View reviewed changes
Source/ASButtonNode.mm Outdated Show resolved Hide resolved
Source/ASButtonNode.mm Outdated Show resolved Hide resolved
Source/ASTextNode.mm Outdated Show resolved Hide resolved
Tests/ASButtonSnapshotTests.mm Outdated Show resolved Hide resolved
rahul-malik
rahul-malik commented Jul 31, 2019
View reviewed changes
Copy link
Contributor Author

@rahul-malik rahul-malik left a comment

Choose a reason for hiding this comment

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

Just took a first pass and there are some obvious things to clean up.

When testing against UILabel, UIButton it appears that

  • UILabel doesn't respond to tintColor directly even if it's set
  • UIButton responds to tintColor but will not override a color that a user has specified

Source/ASTextNode.mm Outdated Show resolved Hide resolved
Tests/ASButtonSnapshotTests.mm Outdated Show resolved Hide resolved
Tests/ASImageNodeSnapshotTests.mm Outdated Show resolved Hide resolved
Source/ASTextNode.mm Outdated Show resolved Hide resolved
rcancro
rcancro reviewed Jul 31, 2019
View reviewed changes
Source/ASButtonNode.mm Show resolved Hide resolved
Source/ASButtonNode.mm Outdated
{
if (title) {
NSMutableAttributedString *mutString = [[NSMutableAttributedString alloc] initWithAttributedString:title];
NSRange limit = NSMakeRange(0, _normalAttributedTitle.length);
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 title, not _normalAttributedTitle? It looks like several different titles are being passed in here in updateTitleForegroundColor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch!

Source/ASButtonNode.mm Outdated Show resolved Hide resolved
nguyenhuy
nguyenhuy reviewed Jul 31, 2019
View reviewed changes
Source/ASImageNode.mm Outdated
|| renderingMode == UIImageRenderingModeAutomatic)
&& key.tintColor) {
asimagenode_modification_block_t tintModificationBlock = ASImageNodeTintColorModificationBlock(key.tintColor);
result = tintModificationBlock(result);
Copy link
Member

@nguyenhuy nguyenhuy Jul 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I like this approach. Using the existing ASImageNodeTintColorModificationBlock helps to encapsulate this drawing operation nicely and makes the code much cleaner. There are perf and memory trade-offs because we draw one image more (compare to rendering the tint color in the first pass), but we can optimize it later if we find the need to.

@ghost
Copy link

ghost commented Aug 2, 2019

🚫 CI failed with log

@ghost
Copy link

ghost commented Aug 8, 2019

🚫 CI failed with log

@rahul-malik
Copy link
Contributor Author

@nguyenhuy @bolsinga - Please take another look.

I've reworked the PR in a few important ways:

  • Tinting for ASTextNode is now handled at the ASTextKitRenderer level
  • More snapshot tests to make sure changing the tint color works correctly
  • Migrate logic in ASButtonNode from setTintColor to tintColorDidChange. We can rely on UIView to call that method instead of overriding the setter
  • Tinting in ASImageNode is now done inline. Code is duplicated but is fairly small and allows us to use the theoretically more performant code path (still needs measurement)
  • Updated headers of new files for proper licensing

@ghost
Copy link

ghost commented Aug 8, 2019

🚫 CI failed with log

@ghost
Copy link

ghost commented Aug 8, 2019

🚫 CI failed with log

@ghost
Copy link

ghost commented Aug 9, 2019

🚫 CI failed with log

@ghost
Copy link

ghost commented Aug 9, 2019

🚫 CI failed with log

@ghost
Copy link

ghost commented Aug 9, 2019

🚫 CI failed with log

nguyenhuy
nguyenhuy approved these changes Aug 12, 2019
View reviewed changes
Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

Nits aside, LGTM!

Source/ASImageNode.mm Outdated Show resolved Hide resolved
Source/ASImageNode.mm Outdated Show resolved Hide resolved
@nguyenhuy
Copy link
Member

To other reviewers: I'm going to land this diff now so we can start testing it internally (we have a fixed sync schedule now). Feel free to do a post-review, especially the changes in ASTextNode2, and we'll follow up on any issues found.

nguyenhuy
nguyenhuy reviewed Aug 12, 2019
View reviewed changes
Source/ASImageNode.mm Outdated Show resolved Hide resolved
@nguyenhuy nguyenhuy merged commit 540f134 into master Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rcancro rcancro rcancro left review comments

@nguyenhuy nguyenhuy nguyenhuy approved these changes

@bolsinga bolsinga bolsinga requested changes

@maicki maicki Awaiting requested review from maicki

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rahul-malik @nguyenhuy @rcancro @bolsinga