Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate Image URL with ImageOptions #66

Merged
merged 8 commits into from May 31, 2017
Merged

Conversation

loudmouth
Copy link
Contributor

Implement tests.

Throw errors for invalid url construction for Images API

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 85.651% when pulling 934bf73 on feature/images-api into c88c7d0 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 85.651% when pulling f49c5d4 on feature/images-api into c88c7d0 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 85.804% when pulling 34c94cb on feature/images-api into c88c7d0 on develop.

@loudmouth loudmouth force-pushed the feature/images-api branch 2 times, most recently from d33b10c to cca76e7 Compare May 31, 2017 11:03

public struct FileMetadata: ImmutableMappable {

let fileName: String
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs documentation.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 85.804% when pulling 163b412 on feature/images-api into c88c7d0 on develop.

Copy link
Contributor

@mariobodemann mariobodemann left a comment

Choose a reason for hiding this comment

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

Some imaginary questions.

@@ -12,6 +12,40 @@ import ObjectMapper
/// An asset represents a media file in Contentful
public class Asset: Resource, LocalizedResource {

/// URL of the media file associated with this asset. Optional for compatibility with `select` operator queries.
public var urlString: String?
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, do you have any plans on also supporting the upload url field? (i.e. the one used while an asset is processing?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is CDA only so far. But, i suppose in the future...yes? I'm only implementing what is listed here

return URL
}
public func url() throws -> URL {
guard let urlString = self.urlString else { throw SDKError.invalidURL(string: "") }
Copy link
Contributor

Choose a reason for hiding this comment

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

That can have a nasty side effect if the asset is uploading, doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is CDA only...i'm not sure i understand what you are talking about.

*/
public func fetchData(for asset: Asset) -> Observable<Result<Data>> {
public func fetchData(for asset: Asset, with imageOptions: [ImageOption] = []) -> Observable<Result<Data>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

😍

// and unique'ify the elements in the array.
let uniqueImageOptions = Array(Set<ImageOption>(imageOptions))
guard uniqueImageOptions.count == imageOptions.count else {
throw SDKError.invalidImageParameters("Cannot specify two instances of ImageOption of the same case."
Copy link
Contributor

Choose a reason for hiding this comment

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

So, actually you can specify two options at the same time: http://images.contentful.com/arqlnkt58eul/2ReMHJhXoAcy4AyamgsgwQ/788a7c2210c923644b4bc20deb9a1f78/lewis-carroll-1.jpg?w=100&w=2000 will return an image with a width of 100.

case .unspecified:
return nil
case .asPercent(let quality):
if quality > 100 {
Copy link
Contributor

Choose a reason for hiding this comment

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

or < 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's an UInt so it's enforced to be >= 0 at compile time

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, okay. I wasn't able to spot the type ... ;)

// If for some reason the following code fails to create a hex string, the color black will be
// returned.
internal func hexRepresentation() -> String {
let hexForBlack = "ffffff"
Copy link
Contributor

Choose a reason for hiding this comment

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

ffffff0? or BlackWhite?

public enum ImageOption: Equatable, Hashable {

/// Specify the size of the image in pixels to be returned from the API. Valid ranges for width and height are between 0 and 4000.
case sizedTo(width: Int, height: Int)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I specify a width only? Aka, I want to scale the image to 100 pixel width, and let the aspect ratio kick in to calculate the height?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 85.809% when pulling 11a3412 on feature/images-api into c88c7d0 on develop.

@loudmouth loudmouth merged commit 7270dad into develop May 31, 2017
@loudmouth loudmouth deleted the feature/images-api branch May 31, 2017 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants