Skip to content

Percent margins - #126

Merged
lucdion merged 4 commits into
layoutBox:masterfrom
vandyshev:percent_margins
Mar 22, 2018
Merged

lucdion merged 4 commits into
layoutBox:masterfrom
vandyshev:percent_margins

Conversation

@vandyshev

Copy link
Copy Markdown
Contributor

Add percent support for margins

@lucdion

lucdion commented Mar 20, 2018

Copy link
Copy Markdown
Member

Thanks @vandyshev for your PR 🎉, I'll check your PR, but don't worry about the failure of travis, I have fixed it in my latest PR

Comment thread Sources/Impl/PinLayoutImpl.swift Outdated

@discardableResult
func marginEnd(_ percent: Percent) -> PinLayout {
return isLTR() ? marginRight(percent) : marginLeft(percent)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Here if there is a warning, the message will display the wrong context (either marginRight or marginLeft), it should display "marginEnd".

Comment thread Sources/Impl/PinLayoutImpl.swift Outdated

@discardableResult
func marginHorizontal(_ percent: Percent) -> PinLayout {
return marginLeft(percent).marginRight(percent)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wrong context here also

Comment thread Sources/Impl/PinLayoutImpl.swift Outdated

@discardableResult
func marginVertical(_ percent: Percent) -> PinLayout {
return marginTop(percent).marginBottom(percent)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

here too, and few other below.

Comment thread Tests/MarginsSpec.swift
}
}

describe("the result of left&right percent margins when only the width is specified") {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Excellent unit tests, could you just add fews in the WarningSpec.

@lucdion lucdion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Excellent work!
Just some small changes required

@vandyshev

Copy link
Copy Markdown
Contributor Author

@lucdion Ok, will be fixed

@lucdion lucdion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good work, thanks for contributing! 👍

@lucdion
lucdion merged commit 902ce67 into layoutBox:master Mar 22, 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.

2 participants