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

Auto layout DSL #210

Merged
merged 5 commits into from
Jan 21, 2020
Merged

Auto layout DSL #210

merged 5 commits into from
Jan 21, 2020

Conversation

ivopintodasilva
Copy link
Contributor

@ivopintodasilva ivopintodasilva commented Dec 18, 2019

Checklist

Motivation and Context

The NSLayoutAnchor API is very verbose and resulted on big blocks of Auto Layout code that increased the size of the view files and was not very easy to read.

As an alternative, Cartography provides a simple and readable DSL to handle Auto Layout code but we've found that the operator overload used on that library was causing our compile times to increase.

Description

All credit goes to @renatorodrigues for the implementation 🙏

The implementation is heavily inspired on Cartography and TinyConstraints.

Without using custom operators, we can still have a more accessible API for our Auto Layout code and avoid the increased build times caused by operator overloads.

@ivopintodasilva
Copy link
Contributor Author

ReusableView unit tests are breaking but #208 is already open to fix them 👼

Copy link
Member

@p4checo p4checo left a comment

Choose a reason for hiding this comment

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

Finished CR, everything looks ace! 🚀

Will wait for the build to pass to check the coverage though 😜

@codecov
Copy link

codecov bot commented Dec 18, 2019

Codecov Report

Merging #210 into master will increase coverage by 0.41%.
The diff coverage is 97.6%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   94.95%   95.36%   +0.41%     
==========================================
  Files          94      101       +7     
  Lines        3408     4034     +626     
==========================================
+ Hits         3236     3847     +611     
- Misses        172      187      +15
Impacted Files Coverage Δ
Sources/AutoLayout/LayoutItem.swift 100% <100%> (ø)
...ources/AutoLayout/NSLayoutConstraint+Helpers.swift 100% <100%> (ø)
Sources/AutoLayout/Array+ConstrainableProxy.swift 100% <100%> (ø)
Sources/AutoLayout/LayoutGuideProxy.swift 87.5% <87.5%> (ø)
Sources/AutoLayout/ViewProxy.swift 89.28% <89.28%> (ø)
Sources/AutoLayout/ConstrainableProxy.swift 98.06% <98.06%> (ø)
Sources/AutoLayout/Constrain.swift 98.18% <98.18%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 463192e...e1a9b17. Read the comment docs.

Copy link
Member

@p4checo p4checo left a comment

Choose a reason for hiding this comment

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

Left a couple of 💅 comments now that we can take advantage of Swift 5.1's implicit return's, which will make the code look tidier and more compact 😌

The test coverage is a bit lacking though, and if we merge this as is we will drop the project's overall coverage by 10.83% 😢, so I think we should add some more tests to address this. I'll gladly help if you want.

Sources/AutoLayout/Constrain.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/LayoutGuideProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/ViewProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/ViewProxy.swift Outdated Show resolved Hide resolved
Copy link

@renatorodrigues renatorodrigues left a comment

Choose a reason for hiding this comment

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

Looks very good. Most of the comments are minor improvements, but there are two important issues:

  1. Unnecessary (duplicated) calls of prepare.
  2. Two helper functions on array that can crash.

Sources/AutoLayout/ConstrainableProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/Array+ConstrainableProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/Array+ConstrainableProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/Array+ConstrainableProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/Array+ConstrainableProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/Array+ConstrainableProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/Array+ConstrainableProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/ViewProxy.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@portellaa portellaa left a comment

Choose a reason for hiding this comment

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

well done 👏

just 💅 and 💄

Sources/AutoLayout/LayoutGuideProxy.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/LayoutItem.swift Outdated Show resolved Hide resolved
Sources/AutoLayout/ViewProxy.swift Outdated Show resolved Hide resolved
Ivo Silva added 2 commits January 16, 2020 14:10
- Internally using `NSLayoutConstraints`
- Constrainable proxies created for a more "suggary" and readable syntax
- Added Array extensions to handle bulk constraint application
- Added unit tests
@ivopintodasilva
Copy link
Contributor Author

Applied code review suggestions and rebased branch.

Currently improving test coverage 📈

@ivopintodasilva
Copy link
Contributor Author

Code coverage replenished 😌

Copy link

@renatorodrigues renatorodrigues left a comment

Choose a reason for hiding this comment

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

🚀

Copy link
Member

@p4checo p4checo 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! 🎉
Small comment regarding API naming 🚓

Sources/AutoLayout/ConstrainableProxy.swift Outdated Show resolved Hide resolved
@ivopintodasilva ivopintodasilva merged commit 2564f28 into master Jan 21, 2020
@ivopintodasilva ivopintodasilva deleted the feature/add-auto-layout-dsl branch January 21, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants