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

Add more paragraph style attributes #104

Merged
merged 6 commits into from
Feb 10, 2016

Conversation

nuudles
Copy link
Contributor

@nuudles nuudles commented Dec 28, 2015

This commit adds several more attributes from the NSParagraphStyle properties, including the firstLineHeadIndent, headIndent, tailIndent, maximumLineHeight, minimumLineHeight, paragraphSpacing, and paragraphSpacingBefore properties. Unit tests have also been added to test these new properties.

This commit adds several more attributes from the NSParagraphStyle properties, including the firstLineHeadIndent, headIndent, tailIndent, maximumLineHeight, minimumLineHeight, paragraphSpacing, and paragraphSpacingBefore properties. Unit tests have also been added to test these new properties.
@ZevEisenberg
Copy link
Collaborator

@nuudles ❤️ ❤️ ❤️ for the time you're putting into this! I'm going to be very busy until just after the new year, so I won't be able to give this a good look until then - sorry for the wait. Two things jump out after a quick peek, though:

  1. It would be nice to differentiate between paragraphSpacingBefore and paragraphSpacingAfter. Apple's API isn't as clear as it could be, but we can use this opportunity to make it better.
  2. BonMot's indentSpacer property uses headIndent internally to do its thing. Have you looked at cases where this might conflict with your headIndent property? Try it out in IndentationCell.m in the example project and see if you run into any interesting edge cases worth testing or throwing exceptions for.

@nuudles
Copy link
Contributor Author

nuudles commented Dec 28, 2015

@ZevEisenberg Congrats dude! No worries on any delay, I know what it's like to plan one of those. I'll take a look at your initial feedback.

Cheers!

@ZevEisenberg
Copy link
Collaborator

@nuudles this all looks great, aside from my earlier comments.

Changing the paragraphSpacing to paragraphSpacingAfter to be clearer as to which paragraph spacing this affects.
@nuudles
Copy link
Contributor Author

nuudles commented Feb 1, 2016

@ZevEisenberg Sorry for the delay in taking a look at this. Work got away from me. I've updated the name of paragraphSpacing to paragraphSpacingAfter. As far as the headIndent property, I spent some time with playing around with the indentSpacer property with headIndent and it seems that the indentSpacer always overwrites headIndent, which is probably as expected. Do you think we should add some documentation to that effect though so that the behavior is documented?

@ZevEisenberg
Copy link
Collaborator

@nuudles A little inline documentation would be appreciated. And if you've got the time, a unit test to make sure that behavior stays consistent. Thanks for getting back to this - always appreciated!

typedef BONChain * (^BONChainLineSpacing)(CGFloat lineSpacing);
typedef BONChain * (^BONChainParagraphSpacing)(CGFloat paragraphSpacing);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please rename the block typedef to match.

It now matches the renamed paragraphSpacingAfter property.
This test case attempts to cover the behavior when you have both the headIndent and indentSpacer set.
@nuudles
Copy link
Contributor Author

nuudles commented Feb 1, 2016

@ZevEisenberg I've tried to add initial tests and documentation as requested regarding the headIndent and indentSpacer behavior. Unfortunately, I'll have limited time in the near future coming back to this if there are additional changes needed.

As a side note, going back to writing Obj C code after dealing with Swift for so long is harder than anticipated.

@ZevEisenberg
Copy link
Collaborator

@nuudles thanks. I'll take it from here if any additional changes need to be made. And I agree about writing Obj-C. I've only been doing Swift in a hobby project, but it's completely ruined me for remembering to put @ on NSString literals.

@ZevEisenberg ZevEisenberg merged commit 45c642a into Rightpoint:master Feb 10, 2016
@ZevEisenberg
Copy link
Collaborator

@nuudles I made a couple of tweaks (1aa95e3...f1dfff2 - nothing major) and merged. Thanks for your contribution!

@nuudles
Copy link
Contributor Author

nuudles commented Feb 10, 2016

@ZevEisenberg That's great! Thanks for implementing the tweaks for me!

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.

2 participants