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

Add background modifier #734

Merged
merged 8 commits into from
Apr 14, 2023
Merged

Add background modifier #734

merged 8 commits into from
Apr 14, 2023

Conversation

supernintendo
Copy link
Contributor

@supernintendo supernintendo commented Mar 24, 2023

Adds support for the background modifier and adds the ability to use namespaced child elements when decoding modifiers. This is based on @carson-katri's idea for handling nested views within modifiers via Content Entities as described here.

<VStack id="background">
  <Spacer />
  <HStack id="example_heart">
    <Image system-name="heart.fill" modifiers={@native |> background(alignment: :center, content: :heart_bg) |> foreground_style(primary: {:color, :white})}>
      <background:heart_bg>
        <Circle modifiers={@native |> frame(width: 32, height: 32) |> foreground_style(primary: {:color, :red})} />
      </background:heart_bg>
    </Image>
  </HStack>
  <Spacer />
  <HStack id="example_liveviewnative">
    <Text modifiers={@native |> background(alignment: :center, content: :text_bg) |> foreground_style(primary: {:linear_gradient, %{gradient: {:colors, [:orange, :indigo, :purple]}, start_point: {0, 0}, end_point: {1, 1}}})}>
      liveviewnative
      <background:text_bg>
        <RoundedRectangle corner-radius="8" modifiers={@native |> frame(width: 192, height: 64)} />
      </background:text_bg>
    </Text>
  </HStack>
  <Spacer />
</VStack>

Screenshot 2023-03-23 at 5 58 14 PM

There are a few outstanding issues with this PR:

  1. The foreground_style modifier influences the background content instead of the background_style modifier which doesn't appear to do anything. I'm not sure why this is but from what I can tell the native backgroundStyle modifier only affects nested content rendered using the in: constructor and not content passed to content.

  2. There seem to be performance limitations with nested modifier views in my implementation. By rendering too many elements with background modifiers within the same view I'm able to produce a crash, although I haven't investigated this enough to know what might be causing it.

  3. The string format for content when referencing namespaced child views could be a bit confusing. I'd like to tweak the Elixir modifier schema so that it accepts an atom instead, which should ameliorate this. Fixed in 2ccb51e

Closes #183.

Copy link
Contributor

@shadowfacts shadowfacts left a comment

Choose a reason for hiding this comment

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

Something is definitely weird with how the background/foreground modifiers are interacting, though it's not obvious to me what the issue is.

In plain old SwiftUI, the following (translated directly from your example) results in just a red circle appearing, since the foreground style is applying to both the Circle and the Image.

Image(systemName: "heart.fill")
	.background(alignment: .center) {
		Circle()
			.frame(width: 32, height: 32)
	}
	.foregroundStyle(.red)

To get the result that you actually show, you'd need to move the foreground style inside the background modifier so that it only applies to the Circle:

Image(systemName: "heart.fill")
	.background(alignment: .center) {
		Circle()
			.frame(width: 32, height: 32)
			.foregroundStyle(.red)
	}

Sources/LiveViewNative/ViewTree.swift Outdated Show resolved Hide resolved
@carson-katri
Copy link
Contributor

Image does not respect the foreground_style modifier because it uses a foregroundColor modifier in its body, which uses the system foreground color when set to nil. The example below produces the same result as the provided screenshot:

Image(systemName: "heart.fill")
            .foregroundColor(nil)
            .background(alignment: .center) {
                Circle()
                    .frame(width: 32, height: 32)
            }
            .foregroundStyle(.red)

The attribute should probably be removed from Image.

Text has the same problem. The textColor, font, and fontWeight attributes should likely be replaced with modifiers as well.

@carson-katri
Copy link
Contributor

RE the background_style modifier, this would require using the BackgroundStyle shape style. Background content created in this way uses the foreground style by default.

@shadowfacts
Copy link
Contributor

shadowfacts commented Mar 24, 2023

Ah, that is indeed the case. We do probably want general modifiers for them, but I think the foregroundColor, font, and fontWeight modifiers need to remain attributes on the Text view as well, since doing them through our usual modifiers architecture would break composing Texts with different styles.

I think a workaround would be having helpers like foregroundColorIfPresent for Text that only apply the underlying modifiers if the value is non-nil, rather than applying .foregroundColor(nil) as currently happens.

@supernintendo
Copy link
Contributor Author

supernintendo commented Apr 1, 2023

@shadowfacts I added foregroundColorIfPresent to fix that in f7e7172. Let me know if there are other examples of this I should address, although maybe that's out of scope for this pull request?

@carson-katri
Copy link
Contributor

@supernintendo I think Text is the only other one with this problem:

public var body: SwiftUI.Text {
text
.font(effectiveFont)
.foregroundColor(textColor)
}

Copy link
Contributor

@shadowfacts shadowfacts left a comment

Choose a reason for hiding this comment

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

This looks good, just needs a slight tweak to make the docs visible

@AZholtkevych AZholtkevych linked an issue Apr 7, 2023 that may be closed by this pull request
2 tasks
@supernintendo supernintendo merged commit 91c3f3f into main Apr 14, 2023
@supernintendo supernintendo deleted the background-modifier branch April 14, 2023 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants