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

Fix wrap content calculation by filter hidden views #280

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ruslandzhafarov
Copy link

No description provided.

@ruslandzhafarov ruslandzhafarov deleted the patch-1 branch March 28, 2024 16:55
@ruslandzhafarov ruslandzhafarov restored the patch-1 branch March 28, 2024 16:56
Sources/PinLayout+WrapContent.swift Outdated Show resolved Hide resolved
@ruslandzhafarov ruslandzhafarov requested a review from lucdion March 28, 2024 18:22
let subviews: [PinView.PinView]
switch viewFilter {
case .visibleOnly:
subviews = view.subviews.filter { !$0.isHidden }
Copy link
Contributor

Choose a reason for hiding this comment

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

What about views with alpha = 0?

There is public visible(_ views: [UIView]) views filter that takes alpha into account -

return views.filter({ !$0.isHidden && $0.alpha > 0 })
, it may lead to confusion if name "view filter" will behave differently here.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, you could use the visible() here to filter views.

}

private func wrapContent(_ type: WrapType, padding: PEdgeInsets, _ context: Context) -> PinLayout {
let subviews = view.subviews
private func wrapContent(_ type: WrapType, padding: PEdgeInsets, viewFilter: ViewFilter = .none, _ context: Context) -> PinLayout {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need default argument value (viewFilter = .none) in private function?

Copy link
Member

Choose a reason for hiding this comment

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

Not needed here, the parameter should always be specified by callers

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. Could you update the documentation of wrapContent (README.md)? 🙏

let subviews: [PinView.PinView]
switch viewFilter {
case .visibleOnly:
subviews = view.subviews.filter { !$0.isHidden }
Copy link
Member

Choose a reason for hiding this comment

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

Good point, you could use the visible() here to filter views.

Sources/Types.swift Outdated Show resolved Hide resolved
@ruslandzhafarov ruslandzhafarov requested a review from lucdion March 28, 2024 23:07
Sources/Extensions/CALayer+PinLayout.swift Outdated Show resolved Hide resolved
@@ -33,6 +33,10 @@ extension NSView: Layoutable {
return PinLayout(view: self, keepTransform: false)
}

public var isVisible: Bool {
!isHidden && alphaValue > 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!isHidden && alphaValue > 0
return !isHidden && alphaValue > 0

@@ -37,6 +37,10 @@ extension UIView: Layoutable, SizeCalculable {
return PinLayoutObjCImpl(view: self, keepTransform: true)
}

public var isVisible: Bool {
!isHidden && alpha > 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
!isHidden && alpha > 0
return !isHidden && alpha > 0

Sources/Types.swift Outdated Show resolved Hide resolved
Sources/Layoutable.swift Outdated Show resolved Hide resolved
@winnisx7
Copy link

bump
why not merge?

Force rebuild
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.

4 participants