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

support keyboardLayoutGuide #238

Merged
merged 12 commits into from
Feb 1, 2022
Merged

support keyboardLayoutGuide #238

merged 12 commits into from
Feb 1, 2022

Conversation

baekteun
Copy link
Contributor

No description provided.

Copy link
Member

@lucdion lucdion left a comment

Choose a reason for hiding this comment

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

Thanks for this addition. Could you please document this new property in the section https://github.com/layoutBox/PinLayout#safearea-readable-and-layout-margins 🙏

@baekteun
Copy link
Contributor Author

Oh?

@@ -83,6 +83,14 @@ public class PinLayout<View: Layoutable> {
}
apply()
}

#if os(iOS)
public var keyboardLayout: PEdgeInsets {
Copy link
Member

Choose a reason for hiding this comment

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

After more thinking, it would better match other properties if we would name it keyboardMargins

  • UIView.readableContentGuide is named pin.readableMargins
  • UIView.layoutMarginsGuide is named pin.layoutMargins

Then I would propose

  • UIView.keyboardLayoutGuide named pin.keyboardMargins

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

@baekteun
Copy link
Contributor Author

"value of type 'UIView' has no member 'keyboardLayoutGuide'".?

@baekteun
Copy link
Contributor Author

Umm... Is it because of the version?

@lucdion
Copy link
Member

lucdion commented Jan 31, 2022

This is strange because Travis is using osx_image: xcode13 and this build step xcodebuild build -project PinLayout.xcodeproj -scheme PinLayout-iOS -sdk iphonesimulator15.0 -destination 'platform=iOS Simulator,name=iPhone 8,OS=15.0'

So this should work. Does it compiles correctly on your system?

@baekteun
Copy link
Contributor Author

Looking at Travis, I think the version of Xcode is 12.5.
스크린샷 2022-02-01 오전 2 43 10

Looking at build-ci.sh, I think the version of OS is 14.5.
스크린샷 2022-02-01 오전 2 44 16

Could you check it again?

@lucdion
Copy link
Member

lucdion commented Jan 31, 2022

Oups, sorry, I forget to open the PR. Locally I had those modifications. I just opened this PR #239

@baekteun
Copy link
Contributor Author

Phew

@lucdion
Copy link
Member

lucdion commented Jan 31, 2022

OK, I have merged #239, you can update your branch.

@baekteun
Copy link
Contributor Author

🛌

@@ -83,6 +83,21 @@ public class PinLayout<View: Layoutable> {
}
apply()
}

#if os(iOS)
Copy link
Member

Choose a reason for hiding this comment

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

Detail, but you could move this block just below public var layoutMargins: PEdgeInsets, so the proprities are declared from the most useful property order? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Member

@lucdion lucdion left a comment

Choose a reason for hiding this comment

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

Excellent. Thanks for the addition 👍

@lucdion lucdion merged commit 2e7f584 into layoutBox:master Feb 1, 2022
@lucdion
Copy link
Member

lucdion commented Feb 1, 2022

New release including your change https://github.com/layoutBox/PinLayout/releases/tag/1.10.1
Thanks!

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