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

Picture component #91

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions Sources/Plot/API/HTMLAttributes.swift
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,24 @@ public extension Attribute where Context == HTML.PictureSourceContext {
static func type(_ type: String) -> Attribute {
Attribute(name: "type", value: type)
}

/// A string with a media query describing which image from the `srcset` attribute should be used.
/// - parameter sizes: The sizes used for the of sources that this element should point to.
static func sizes(_ sizes: String) -> Attribute {
Attribute(name: "sizes", value: sizes)
}

/// Assign an integer giving the intinsic height of the `srcset` image in pixels.
/// - parameter height: The height of the image in the source.
static func height(_ height: Int) -> Attribute {
Attribute(name: "height", value: String(height))
}

/// Assign an integer giving the intrinsic width of the `srcset` image in pixels.
/// - parameter width: The width of the image in the source.
static func width(_ width: Int) -> Attribute {
Attribute(name: "width", value: String(width))
}
}

// MARK: - Forms, input and options
Expand Down
56 changes: 56 additions & 0 deletions Sources/Plot/API/HTMLComponents.swift
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,62 @@ extension List: ComponentContainer where Items == ComponentGroup {
self.init(content()) { $0 }
}
}
// Component used to render a `<picture>` element for responsive image support.
public struct Picture: Component {

Copy link
Owner

Choose a reason for hiding this comment

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

Code style nitpick: Please remove this blank line to make this type definition consistent with the rest of the library.

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

/// Image to use as fallback.
public var image: Image
/// A set of `source` elments from which a user agent will chose based on its requirements.
public var sources: [Source]

/// Initialize a `Picture` component with multiple `source` elements.
internal init(image: Image, sources: [Picture.Source] = []) {
Copy link
Owner

Choose a reason for hiding this comment

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

This initializer can also be removed, right? Given that the default, memberwise initializer can be used within the public one. But I'm also thinking if this shouldn't be the default public initializer. Isn't it more common to use multiple sources when using a <picture> element? Otherwise, you might as well just use a plain <img>, or perhaps I'm missing something?

Copy link
Author

Choose a reason for hiding this comment

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

It is more common, but the spec does allow a single source. Examples would be using a srcset attribute with multiple images and a sizes attribute with the required sizes, or using an advanced image type (like AVIF), and a JPG fallback image.

Copy link
Owner

Choose a reason for hiding this comment

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

So, here's what I think we should do:

  1. Make this initializer (the one that takes an array of sources) public, since it covers the majority use case.
  2. Remove the initializer that takes a single source (since that same functionality can easily be achieved by simply passing a single source within an array).
  3. Add parameter documentation to the initializer (similar to what the other built-in components have).

self.image = image
self.sources = sources
}

/// Initialize a `Picture` component with a single `source` element.
/// Image to use as fallback.
/// A set of `source` elments from which a user agent will chose based on its requirements.
public init(image: Image, source: Source) {
self.init(image: image, sources: [source])
}

public var body: Component {
Node.picture(
Copy link
Owner

Choose a reason for hiding this comment

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

Sill incorrect indentation here. Please format your code according to the rest of the library (4 spaces indentation per level) to keep things consistent.

.forEach(sources) { source in
.source(
.srcset(source.sourceSet),
.media(source.media ?? ""),
.type(source.type ?? ""),
.sizes(source.sizes ?? ""),
.unwrap(source.height, Attribute.height),
.unwrap(source.width, Attribute.width)
)
},
.component(image)
)
}
}

public extension Picture {
/// Type used to define a Picture source, which points to an image file (or set of them) and associated media queries (and other paramaters).
struct Source {
Copy link
Owner

Choose a reason for hiding this comment

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

This struct needs an explicit public initializer, since otherwise you won't be able to initialize it from outside the Plot library (I know I told you to remove the initializer before, but that was when you had defined it as internal, which wouldn't be needed).

/// Image or images to use - can have parameters
public var sourceSet: String
/// Sizes for the `srcset` to use - can have parameters
public var sizes: String?
/// Media querry (optional)
public var media: String?
/// Media type (optional)
public var type: String?
/// Intrinsic height of image in pixels - integer with no unit (optional)
public var height: Int?
/// Intrinsic width of image in pixels - integer with no unit (optional)
public var width: Int?
}

}

/// Convenience type that can be used to create an `Input` component
/// for submitting an HTML form.
Expand Down