twostraws / Ignite

A static site generator for Swift developers.
MIT License
985 stars 34 forks source link

Feature Request: allow for custom positioning of Card Content Overlay #31

Closed TheApApp closed 1 week ago

TheApApp commented 2 weeks ago

Positioning of overlay

Card(imageName: "/images/wind.jpg") {
                Text("Arya, Samoyed")
                    .foregroundStyle(.white)
                    .fontWeight(.bold)
            }
            .contentPosition(.overlayCenter) // possible solution? 
pd95 commented 2 weeks ago

I was looking into this problem and found the following solution to position the text in the middle of the card:

  1. Make the text take up maximum space
  2. Add the two classes "text-center" (for horizontal-alignment) and "align-content-center" (for vertical-alignment)
Card(imageName: "/images/wind.jpg") {
    Text("Arya, Samoyed")
        .foregroundStyle(.white)
        .fontWeight(.bold)
        .frame(width: "100%", height: "100%")
        .class("text-center", "align-content-center")
}
.contentPosition(.overlay)

Now it would probably be nice to fit everything into a nice API... something like a ZStack with alignment properties 😃

twostraws commented 2 weeks ago

GitHub automatically marked this as being complete when the PR was merged, but I think it's worth leaving it open until we're sure it's fully solved. Specifically, are we happy that overlayCenter is the right approach, vs using overlay then a separate modifier, or a second parameter? Or perhaps even .overlay(alignment:)? My concern is that someone might want a center horizontal alignment, and a bottom vertical alignment, which isn't really covered by the new API.

pd95 commented 2 weeks ago

You are right: .overlay(alignment:) would solve the various alignments, without the need for a ZStack 😉 I could prepare this, if you like.

twostraws commented 2 weeks ago

Sounds good – although I'd be interested to see how much we can retain code backward-compatibility.

pd95 commented 2 weeks ago

I've created a prototype with an additional modifier and a new enum ContentAlignment.

enum ContentAlignment: CaseIterable {
    case topLeading
    case top
    case topTrailing
    case leading
    case center
    case trailing
    case bottomLeading
    case bottom
    case bottomTrailing
}

extension Card {
    func contentAlignment(_ newAlignment: ContentAlignment) -> Self {
        // more code ...
    }
}

The positioning in the overlay is triggered by the Bootstrap CSS classes for text alignment and vertical alignment. (=the same properties used for the current implementation of .overlayCenter)

Bildschirmfoto 2024-04-29 um 21 52 21
Section {
    for alignment in Card.ContentAlignment.allCases {
        let alignmentName = String(describing: alignment)
        Card(imageName: "/images/photos/dishwasher.jpg") {
            Text(markdown: "`.\(alignmentName)`")
                .foregroundStyle(.white)
                .backgroundColor(.white)
        }
        .contentPosition(.overlay)
        .contentAlignment(alignment)
        .imageOpacity(0.5)
    }

}
.margin(.top, .large)
.columns(3)
pd95 commented 2 weeks ago

Pushed into a "draft PR": https://github.com/twostraws/Ignite/pull/37

pd95 commented 2 weeks ago

With 873017c8f729cc0bec5f19a756d37d42c125bfc3 I use "flex" to do the same thing... Gives a nicer result, as the text is not spread out over the whole line:

Bildschirmfoto 2024-04-29 um 22 20 38
twostraws commented 2 weeks ago

With this being used as a separate modifier rather than overlay(alignment:) or similar, does this set a user expectation that contentAlignment() can be used without contentPosition() – does that even work sensibly?

pd95 commented 2 weeks ago

You are right. To be on the safe side, it would make sense to name it overlayAlignment... or make a breaking change and create the overlay(alignment:) variant.

I quickly tested the content alignment prototype combined with ContentPosition.bottom. It can work... but it has some problems. Perhaps due to my bad example using texts with multiple lines side-by-side?

Card(imageName: "/images/photos/dishwasher.jpg") {
    Text("A<br/>B<br/>C<br/>D")
        .backgroundColor(.yellow)
    Text(markdown: "`.bottomTrailing`")
        .foregroundStyle(.white)
        .backgroundColor(.lightGray)

    Text("One<br/>Two<br/>Three")
        .backgroundColor(.seaGreen)
}
.contentAlignment(.bottomTrailing)
.imageOpacity(0.5)

The alignment "bottom" and "trailing" is visible, as the Text fields are aligned accordingly in the content area... but they are not "bottom" aligned, because each

tag followed by another

tag has an automatic margin set, as shown in the capture below.

ContentAlignment-Text-Problem

NigelGee commented 2 weeks ago

While we are aligning the .overlay text. What about .offset modifier

I would would also like to add that at the moment it can put the text .bottom, .top, .overlay but could it have the text .trailing and .leading (which would snap to .bottom if .trailing and .top if .leading when size get too small).

Do you want a separate issue for this?

Sorry do not know enough about HTML coding to do a PR.

twostraws commented 2 weeks ago

@pd95 Given the options, I'd prefer breaking backwards compatibility and implement .overlay(alignment: .top); I can just update my existing tutorial to mention this. That feels more natural to me, and also causes less confusion about modifier combinations. Does that sound good to you?

@NigelGee Offsets would be a separate issue.

pd95 commented 2 weeks ago

@twostraws : I've reworked my solution and documented it in #37 . It uses now .contentPosition(.overlay(alignment: .center)) for centering the overlay. I've added overlay as a static variable to ContentPosition for backward compatibility. But I removed .overlayCenter as this had been included previously "by accident" and was not part of a release.

Let me know whether it needs more rework. I will add a separate PR for IgniteSample later.

twostraws commented 1 week ago

No further work needed; this is a really great improvement. Thank you!