twostraws / Ignite

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

Enhancing `Card` #32

Closed pd95 closed 2 weeks ago

pd95 commented 2 weeks ago

Inspired by your HWS+ session about Ignite today, I was looking more into how Card is working and how the generated HTML relates to the Bootstrap documentation. I've seen that it would be possible to place the image above and below the content text and that Bootstrap is "capping" the edges of the image accordingly. The current version of ContentPosition does only support placing the content below the image (=first image, then content) and hardcoded the CSS classes accordingly to "card-img-top".

I've enhanced this by adding additional options. Further I added a fix to allow the image being placed into content of a Card: The image has to get the "card-img" CSS class for proper sizing.

I'm not yet fully convinced by the current solution. Do you think ContentPosition enum needs more adjustments? Is ContentPosition the correct wording? Wouldn't ImagePosition .top, .bottom and .background be more appropriate?

Screenshot showing all 4 variations

Source code for above screenshot:

Section {
    Card(imageName: "/images/rug.jpg") {
        Text(markdown: "Content below image: `.bottom`")
    }
    .contentPosition(.bottom)

    Card(imageName: "/images/rug.jpg") {
        Text(markdown: "Content before image: `.top`")
    }
    .contentPosition(.top)

    Card(imageName: "/images/rug.jpg") {
        Text(markdown: "Content overlaid over image: `.overlay`")
            .foregroundStyle(.white)
            .padding()
            .backgroundColor(.gray.opacity(0.5))
    }
    .contentPosition(.overlay)
    .margin(.top, .medium)

    // A card containting an image within the body
    Card {
        Text("Image embedded")
        Image(decorative: "/images/rug.jpg")
        Text("as part of the card")
    }
    .margin(.top, .medium)
}
.columns(2)
.margin()
twostraws commented 2 weeks ago

First: thank you for this contribution! Could you share the Ignite code used to generate your examples, please? It helps everyone see how this all looks at the call site.

Second: I'm certainly not committed to the current spelling, and am happy to bounce around others until we find something that works well 🙂 Generally speaking, what I'm keen to avoid are modifiers that mean different things in different places, and in this case my first preference would have been cardStyle() because it's clear that this is a Card-specific modifier. Sadly, that was taken by existing card styling code, so I ended up with contentPosition(). contentPosition() seemed a little less common to me than imagePosition(), but both aren't used so both seem sensible choices. (Note: If this changes we should retain the existing contentPosition() modifier and mark it as deprecated, to avoid breaking code.)

Wherever we end up, it would be helpful to make sure it aligns well with any future work done to accommodate #31.

pd95 commented 2 weeks ago

The change I've added is just a possible implementation of .overlayCenter. I'm wondering whether it would make more sense to work on HStack, VStack and ZStack for more control over positioning, instead of building some alignment options into each possible supported element.

@twostraws : You have probably have some idea on how to handle the alignment options or even did some experimentation with stacks already?

twostraws commented 2 weeks ago

Thank you for clarifying how the code is used! I'll merge this change in now. There's still scope to discuss spelling of this API in the future, but at least folks can benefit from this now rather than waiting further.

SwiftUI's stacks are tricky, because they make a promise I don't particularly want to keep 😅

If you say HStack { View() View() View() }, SwiftUI will place those views horizontally. That might be fine when you're building for iPhone, but on the web really we'd want that HStack to wrap to two lines or even three when space is restricted. That's exactly how Section works, which is why I gave it a different name from HStack – I don't want folks to consider it a "false friend", in language terms.

That being said, we could still add a VStack and even a ZStack, leveraging parts or all of Bootstrap's flex utilities. That could include a HStack, effectively making a Section that never wraps, but again I think we ought to tread carefully there.

twostraws commented 2 weeks ago

@pd95 When you have a moment, could you open a PR on IgniteSamples showing off the new functionality with some sample code? Thank you!