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

[ASTextNode] Fixes in ASTextKitFontSizeAdjuster #1056

Merged
merged 2 commits into from
Aug 15, 2018
Merged

[ASTextNode] Fixes in ASTextKitFontSizeAdjuster #1056

merged 2 commits into from
Aug 15, 2018

Conversation

ejensen
Copy link
Contributor

@ejensen ejensen commented Jul 31, 2018

  • Removes double scaling of paragraphSpacing.
  • Removes scaling of lineHeightMultiple attribute since it is a scale of the lineHeight which is already being adjusted.
  • Adds Unit tests and Screenshot tests for ASTextKitFontSizeAdjuster.

diff

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2018

CLA assistant check
All committers have signed the CLA.

@@ -87,8 +87,6 @@ + (void)adjustFontSizeForAttributeString:(NSMutableAttributedString *)attrString
paragraphStyle.tailIndent = (paragraphStyle.tailIndent * scaleFactor);
paragraphStyle.minimumLineHeight = (paragraphStyle.minimumLineHeight * scaleFactor);
paragraphStyle.maximumLineHeight = (paragraphStyle.maximumLineHeight * scaleFactor);
paragraphStyle.lineHeightMultiple = (paragraphStyle.lineHeightMultiple * scaleFactor);
paragraphStyle.paragraphSpacing = (paragraphStyle.paragraphSpacing * scaleFactor);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

paragraphSpacing is already being scaled in line 84. Repeating the scaling cases the paragraph spacing mismatch the font point scaling.

@@ -2218,36 +2219,6 @@
shellScript = "diff \"${PODS_PODFILE_DIR_PATH}/Podfile.lock\" \"${PODS_ROOT}/Manifest.lock\" > /dev/null\nif [ $? != 0 ] ; then\n # print error to STDERR\n echo \"error: The sandbox is not in sync with the Podfile.lock. Run 'pod install' or update your CocoaPods installation.\" >&2\n exit 1\nfi\n# This output is used by Xcode 'outputs' to avoid re-running this script phase.\necho \"SUCCESS\" > \"${SCRIPT_OUTPUT_FILE_0}\"\n";
showEnvVarsInLog = 0;
};
3B9D88CDF51B429C8409E4B6 /* [CP] Copy Pods Resources */ = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not run the tests due to the Cocoapods build script throwing an error. The error message recommended running pod install. The resulting change was the removal of these two run script phases. With them gone, I can now run the tests.

Copy link
Member

Choose a reason for hiding this comment

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

Could this be due to differing versions of cocoa pods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what caused it. I have rebased my code changes onto master and the issue went away. So I've reverted this project file change.

Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

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

Great work.

Copy link
Contributor

@maicki maicki left a comment

Choose a reason for hiding this comment

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

Nice!

@nguyenhuy nguyenhuy merged commit 5f912d1 into TextureGroup:master Aug 15, 2018
@ejensen ejensen deleted the FontSizeAdjusterFixes branch August 15, 2018 20:41
mikezucc pushed a commit to mikezucc/Texture that referenced this pull request Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants