danielsaidi / SystemNotification

SystemNotification is a Swift SDK that helps you mimic the native iOS system notification in SwiftUI.
MIT License
428 stars 14 forks source link

SystemNotificationConfiguration.backgroundColor API Question/Suggestion #12

Closed mergesort closed 1 month ago

mergesort commented 1 year ago

Hi Daniel, I'm really enjoying your library! It was incredibly quick to integrate and so well documented, I can see you put a great deal of love into this project.

I was exploring a use case to standardize how I send messages across my app, ostensibly an enum with views pre-populated so I can have simpler call-sites. At it's core it looks something like this.

extension SystemNotificationContext {
    func present(notification: AppNotification, edge: SystemNotificationEdge = .top) {
        self.present(content: notification.view, configuration: .init(backgroundColor: .random, edge: edge))
    }
}

enum AppNotification {
    case authenticated
    case logout

    var view: some View {
        switch self {

        case .authentication:
            SystemNotificationMessage(
                icon: Image.icons.link,
                title: "AUTHENTICATION_SUCCESS_TITLE",
                text: "AUTHENTICATION_SUCCESS_TEXT",
                configuration: .init(iconColor: palette.success)
            )

        case .logout:
            SystemNotificationMessage(
                icon: Image.icons.door,
                title: "LOG_OUT_SUCCESS_TITLE",
                text: "LOG_OUT_SUCCESS_TEXT",
                configuration: .init(iconColor: palette.warning)
            )
     }
}

This wrapper is serving me pretty well, but I think there may be room for a slight improvement, unless I'm misunderstanding the intent (which is entirely possible). After reading through the library's code it's not clear to me why the backgroundColor is defined on SystemNotificationConfiguration, rather than on SystemNotificationMessage.

This is a bit of an issue for me because my colors are pulled from the environment, which I don't have access to in the context of SystemNotificationContext. To remedy that I could pass the backgroundColor into each present call, but that seems like a subpar abstraction and I'd much rather have it defined in SystemNotificationMessage so each instance of AppNotification could define the background color it should be displayed with. I think this should be possible, but I understand I don't have all the context you have from building this library.

Thanks again, looking forward to hearing your thoughts, and please let me know if there's anything I can do to improve my implementation based on what you've built!

danielsaidi commented 1 year ago

Hi @mergesort

Happy to hear that you like the library!

I use the enum approach in my own apps as well, and really like that approach 👍

Regarding the background color, I've had this challenge myself. It basically comes down to the notification container view and its content. Since the content view is inset by a padding that is defined by the notification configuration, any background color applied or provided by the content view will only apply to the content view itself and not the container.

So that's basically the (current) limitation, since a SystemNotificationMessage is just a content view.

However, as you've noticed, you have a custom configuration override that you can provide in the context's present function. This means that you can provide a custom configuration every time you call present.

If you want this custom configuration to be provided by the AppNotification itself, you could add a new property to the enum, that defines the custom notification configuration:

enum AppNotification {

     ....

    var notificationConfiguration: SystemNotificationConfiguration {
         switch self {
             case .authentication: return .init(backgroundColor: ...)
             default: return .standard
         }
    }
}

Would this suffice?

mergesort commented 1 year ago

Thank you so much for the detailed response @danielsaidi! I've taken a look over it and have actually tried to make that work, with a caveat. This is where I confess and say that I omitted an important part of my problem, I need to access the Environment to reference palette colors that I store in there.

I have this struct, which in essence provides me access to an Environment in circumstances where I normally wouldn't.

public struct EnvironmentValueAccessView<Value, Content: View>: View {
    private let keyPath: KeyPath<EnvironmentValues, Value>
    private let content: (Value) -> Content

    @usableFromInline
    @Environment var environmentValue: Value

    public init(_ keyPath: KeyPath<EnvironmentValues, Value>, @ViewBuilder content: @escaping (Value) -> Content) {
        self.keyPath = keyPath
        self.content = content

        self._environmentValue = .init(keyPath)
    }

    public var body: some View {
        content(environmentValue)
    }
}

That means that my view actually looks more like this, having previously omitted EnvironmentValueAccessView for simplicity. The EnvironmentValueAccessView struct allows me to access the color palette, from my environment, which is what palette.success represents.

private extension AppNotification {
    var view: some View {
        EnvironmentValueAccessView(\.preferredColorPalette, content: { palette in
            switch self {

            case .authentication:
                SystemNotificationMessage(
                    icon: Image.icons.link,
                    title: "AUTHENTICATION_SUCCESS_TITLE",
                    text: "AUTHENTICATION_SUCCESS_TEXT",
                    configuration: .init(iconColor: palette.success)
                )
            }
        })
    }
}

Because of that I can't create var notificationConfiguration: SystemNotificationConfiguration and do the same, given EnvironmentValueAccessView outputs a View, which SystemNotificationConfiguration doesn't conform to. (Correctly so of course.) If the background color was on the SystemNotificationMessage itself I would be able to leverage EnvironmentValueAccessView.


I still think it would make sense to expose a backgroundColor on the SystemNotificationMessage, but since I have a lot less experience with this library it makes sense to ask is there a use case where the container and the content background colors should be different? I haven't seen any examples in your demo where that's the case, but if so I figured I'd offer two options that I think could be potential solutions.

  1. Offering two different color choices, a container background color and a message background color. The message background color could fill if there isn't a container background color set, and otherwise the library could default to the way it's currently being done.
  2. Using a preference key to bubble up the message color to the container. This would allow you to set the color on the message, but have it be applied to the container.
  3. I haven't dug into the code enough to make this last suggestion strongly, but could the message color be used to greedily fill all the way out to the container?

I'm sure I'm missing a lot of context so please take all of these with a grain of salt. Thanks again for taking the time to respond, and for the suggestions!

danielsaidi commented 1 year ago

Oh, I see - your setup is a bit more complicated then :)

Since the SystemNotificationMessage is just a view, just like any other custom view that you can use, it will be applied in a SystemNotification container, which has its own default style and configuration. The container will apply intrinsic padding, which means that any color you apply to the message (or any custom view) will not render outside the bounds of the view.

However, when you call present, you can provide the function with a custom style and configuration, which will be used for that presentation alone. Since you have an enum that defines your various notifications, couldn't that enum also resolve an optional custom SystemNotificationStyle for the notifications that needs to use a custom background?

mergesort commented 1 year ago

I actually missed the style parameter on SystemNotificationMessage in the first place, but I don't believe I should be able to access SystemNotificationStyle in the AppNotification enum. I think I only have access to SystemNotificationMessageStyle, like this.

private extension AppNotification {
    var view: some View {
        EnvironmentValueAccessView(\.preferredColorPalette, content: { palette in
            switch self {

            case .authentication:
                SystemNotificationMessage(
                    icon: Image.icons.link.foregroundColor(palette.success),
                    title: "AUTHENTICATION_SUCCESS_TITLE",
                    text: "AUTHENTICATION_SUCCESS_TEXT",
                    style: SystemNotificationMessageStyle(textColor: palette.tertiary)
                )
            }
        })
    }
}

I could add a separate parameter to the present function, but then that would mean passing in the style at every call-site which is what I'm trying to avoid.

extension SystemNotificationContext {
    func present(notification: AppNotification, style: SystemNotificationStyle? = nil) {
        self.present(content: notification.view, style: style)
    }
}

Perhaps I'm overlooking an approach you're thinking of?

danielsaidi commented 1 year ago

I'm having some problem understanding why the current things that are in place are not enough. I will consider changing the api so that you can present "something" that also specifies a notification style and configuration, but if you figure something better out, please let me know.

mergesort commented 1 year ago

Sorry if it's been unclear, I understand that this is difficult to communicate in bits and pieces. I figured it might be best to make a minimal reproducible example of what I'm trying to achieve so I took the Demo project and added three files, AppNotification, Color.Palette, and EnvironmentAccessView. This is almost identical to the setup I have, short of changing some strings, colors, and other properties.

SystemNotification.zip

In this project I have this code to present a notification, and to set it's color (amongst potentially other properties).

extension SystemNotificationContext {
    func present(notification: AppNotification, style: SystemNotificationStyle? = nil) {
        self.present(content: notification.view, style: style)
    }
}

I would actually prefer to read like this, with the AppNotification knowing what background color it should be presented with since a success notification should always use green (palette.success).

extension SystemNotificationContext {
    func present(notification: AppNotification) {
        self.present(content: notification.view)
    }
}

With the current configuration though I'm left to call it like this, including the style parameter to define the color.

context.present(notification: .authentication, style: .init(backgroundColor: palette.success))

The reason I add the style parameter is so I can modify the background color of the SystemNotificationStyle to be palette.success. Since the background color is set through a SystemNotificationStyle not the SystemNotificationMessageStyle, I can't set that in the AppNotification.view. I also can't make another variable in AppNotification to represent the color is pulled from a palette that lives in the Environment, the Environment only being accessible in a View or ViewModifier.

I hope that makes things clearer, and if not, I'd be happy to see if there's any way to provide more info.