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

Allow modified views to fill their parent if a child requires it #165

Merged
merged 2 commits into from
Jul 7, 2020

Conversation

carson-katri
Copy link
Member

For instance, a ModifiedContent view wrapping a VStack with a Spacer will allow the stack to fill the parent's height by putting height: 100%; on itself. This goes for all views that have a child SpacerContainer that fills either axis.

@carson-katri carson-katri added the bug Something isn't working label Jul 7, 2020
@carson-katri carson-katri requested a review from MaxDesiatov July 7, 2020 01:47
@ie-ahm-robox
Copy link
Collaborator

Fails
🚫

Sources/TokamakDOM/DOMRenderer.swift#L81 - Line should be 100 characters or less: currently 127 characters (line_length)

🚫

Sources/TokamakDOM/Shapes/Shape.swift#L21 - Line should be 100 characters or less: currently 125 characters (line_length)

Generated by 🚫 Danger Swift against 8ab706a

@@ -79,6 +79,7 @@ private extension EdgeInsets {
}

extension _PaddingLayout: DOMViewModifier {
public var orderDependent: Bool { true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
public var orderDependent: Bool { true }
public var isOrderDependent: Bool { true }

@@ -19,6 +19,12 @@ public typealias ModifiedContent = TokamakCore.ModifiedContent

public protocol DOMViewModifier {
var attributes: [String: String] { get }
/// Can the modifier be flattened?
var orderDependent: Bool { get }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
var orderDependent: Bool { get }
var isOrderDependent: Bool { get }

}

extension DOMViewModifier {
public var orderDependent: Bool { false }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public var orderDependent: Bool { false }
public var isOrderDependent: Bool { false }

@@ -42,6 +48,7 @@ extension _ZIndexModifier: DOMViewModifier {
}

extension _BackgroundModifier: DOMViewModifier where Background == Color {
public var orderDependent: Bool { true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public var orderDependent: Bool { true }
public var isOrderDependent: Bool { true }

content
})
if let adjacentModifier = content as? AnyModifiedContent,
!(adjacentModifier.anyModifier.orderDependent || domModifier.orderDependent) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
!(adjacentModifier.anyModifier.orderDependent || domModifier.orderDependent) {
!(adjacentModifier.anyModifier.isOrderDependent || domModifier.isOrderDependent) {

@@ -32,6 +32,7 @@ extension _OverlayModifier: DOMViewModifier where Overlay == _ShapeView<_Stroked

// TODO: Implement arbitrary clip paths with CSS `clip-path`
extension _ClipEffect: DOMViewModifier {
public var orderDependent: Bool { true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public var orderDependent: Bool { true }
public var isOrderDependent: Bool { true }

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

This is great, just a nitpick to make boolean property naming consistent.

@carson-katri carson-katri requested a review from MaxDesiatov July 7, 2020 14:18
Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Lovely! 👍

@carson-katri carson-katri merged commit 1f2203d into main Jul 7, 2020
@carson-katri carson-katri deleted the fill-parent-hotfix branch July 7, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants