Skip to content

Conversation

fred-bowker
Copy link
Contributor

No description provided.

@fred-bowker
Copy link
Contributor Author

When moving from beta to production key with the GIPHY API, you are required to display an attribution mark "Powered By Giphy"

https://support.giphy.com/hc/en-us/articles/360035158592-What-conditions-does-my-app-project-need-to-meet-in-order-to-get-a-production-API-Key-

It is not that clear where you need to display this:
buddyboss/buddyboss-platform#1283

This pull request adds the attribution mark to the giphy popup, and is fairly inconspicuous, it is off by default as the app developer may already be displaying the attribution mark somewhere else in the app.

Basically I have had to add this to get through the review process to move from beta to production key

@fred-bowker
Copy link
Contributor Author

@jaesung-0o0 Hi could you have a look at this pull request, many thanks Fred

@x-0o0 x-0o0 self-requested a review October 1, 2023 11:01
@x-0o0 x-0o0 assigned fred-bowker and unassigned fred-bowker Oct 1, 2023
@x-0o0 x-0o0 added the enhancement New feature or request label Oct 1, 2023
Comment on lines +21 to +35
// Targets are the basic building blocks of a package. A target can define a module or a test suite.
// Targets can depend on other targets in this package, and on products in packages this package depends on.
.target(
name: "ChatUI",
dependencies: [
.product(name: "GiphyUISDK", package: "giphy-ios-sdk")
],
resources: [
.process("Resources")
]
),
.testTarget(
name: "ChatUITests",
dependencies: ["ChatUI"]),
]
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
// Targets are the basic building blocks of a package. A target can define a module or a test suite.
// Targets can depend on other targets in this package, and on products in packages this package depends on.
.target(
name: "ChatUI",
dependencies: [
.product(name: "GiphyUISDK", package: "giphy-ios-sdk")
],
resources: [
.process("Resources")
]
),
.testTarget(
name: "ChatUITests",
dependencies: ["ChatUI"]),
]
// Targets are the basic building blocks of a package. A target can define a module or a test suite.
// Targets can depend on other targets in this package, and on products in packages this package depends on.
.target(
name: "ChatUI",
dependencies: [
.product(name: "GiphyUISDK", package: "giphy-ios-sdk")
],
resources: [
.process("Resources")
]
),
.testTarget(
name: "ChatUITests",
dependencies: ["ChatUI"]),
]

Copy link
Owner

Choose a reason for hiding this comment

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

Convention: Indent -> 4 space


struct GiphyAttributionMarkView: View {

private let PADDING_ABOVE_TAB = 2.5
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
private let PADDING_ABOVE_TAB = 2.5
private let paddingAboveTab = 2.5

please modify snake case to lower camel case :)

: "Poweredby_100px-White_VertText"
}
}

Copy link
Owner

Choose a reason for hiding this comment

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

Please add GiphyAttributionMarkView.Previews to Previews/ChatInChannel path:

struct GiphyAttributionMarkView_Preview: PreviewProvider {
    static var previews: some View {
        GiphyAttributionMarkView()
            .preferredColorScheme(.dark)
    }
}

@x-0o0
Copy link
Owner

x-0o0 commented Oct 1, 2023

Hi @fred-bowker :) Sorry for the late check.

What a good catch!
Thanks for the great PR 👍

However, while reviewing the PR, I faced an image unassigned issue and the image doesn't show up on the preview canvas:
Screenshot 2023-10-01 at 8 10 11 PM

Also could you add the previews of the new view:GiphyAttributionMarkView to Previews/ChatInChannel path?

Screenshot 2023-10-01 at 8 20 32 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants