design-ops / sketch-to-swiftUI

23 stars 5 forks source link

Reverse ordering of layer styles #34

Closed kerrmarin closed 3 years ago

kerrmarin commented 3 years ago

The latest Sketch produces the array of layers in the order that they are in the sketch document. Unfortunately, that means that in SwiftUI we need to reverse the applying of the view modifiers to match the sketch doc. For example:

A sketch document with 2 layers: a background and an image on top will be represented in the sketch doc as an array of layers like [color, image] which yields SwiftUI code like:

AnyView($0
    .background(Color(red: 0.95, green: 0.95, blue: 0.95, opacity: 1))
    .background(Image("f1caef7f83c2b6138b3b5397622c73f0a1ef053c", bundle: nil).resizable().aspectRatio(contentMode: .fill))
)

However, in SwiftUI that is the reverse of the ordering we require. This PR reverses the final background array.

There will be forthcoming PRs to do the same with shadows and borders if following some tests it is revealed that they are required to be reversed too.

deanWombourne commented 3 years ago

Should you check the sketch version and reverse accordingly?

kerrmarin commented 3 years ago

@deanWombourne 🤔 My initial reaction would be no; we've never supported different versions of Sketch and it might set a precedent I don't know we want to follow. (I'm also not sure which version of sketch changed this, or if it was always broken -- I just know the latest sketch has this "problem")

@cdemetriadis @atomoil thoughts?

deanWombourne commented 3 years ago

I've approved, just so we aren't slowed down by deciding on backwards compatibility. I guess if anyone needs it they can raise an Issue :)

kerrmarin commented 3 years ago

I spent a bit more time looking into this and there is another way to fix it. The issue is that the SwiftUI code we output is for each background layer:

...
   .background(layerfill1)
   .background(layerfill2)
   .background(layerfill3)

Which is telling SwiftUI to make the view's background layerfill1, then the result of that make its background layerfill2 and the result of that make its background layerfill3. When in reality what we want is to set the background to a combination of layerfill1, 2 and 3. You can achieve the same result with:

...
    .background(layerfill1.overlay(layerfill2).overlay(layerfill3))

Or in a more concrete example: Type 1:

        stylist.addStyle(identifier: "layer-fills") {
            AnyView($0
                        .background(Color(red: 1, green: 0, blue: 0, opacity: 0.3023382867132867))
                        .background(Image("aafa0cd704b4d5e0b90ff133c0582bb46448a7f0", bundle: nil).resizable().aspectRatio(contentMode: .fill))
                        .background(Color(red: 0.85, green: 0.85, blue: 0.85, opacity: 1))
            )
        }

Type 2:

            AnyView($0
                        .background(Color(red: 0.85, green: 0.85, blue: 0.85, opacity: 1)
                                        .overlay(Image("aafa0cd704b4d5e0b90ff133c0582bb46448a7f0", bundle: nil).resizable().aspectRatio(contentMode: .fill))
                                        .overlay(Color(red: 1, green: 0, blue: 0, opacity: 0.3023382867132867))
                        )
           )

With type2 we wouldn't reverse the array we would check if there is more than one background item and "condense them" by overlaying them on each other.

Which one do you think is best?

deanWombourne commented 3 years ago

(without any research at all) Option 2. You're being explicit to SwiftUI what you're trying to do so they can optimise (and probably already have a bit).

kerrmarin commented 3 years ago

I thought you might say that ;)

I'll update the PR tonight

kerrmarin commented 3 years ago

@atomoil @deanWombourne all updated :)