-
Notifications
You must be signed in to change notification settings - Fork 137
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
base: master
Are you sure you want to change the base?
Changes from 4 commits
bc3f53b
f322c98
6da3926
efa1f66
4a0924a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -579,6 +579,72 @@ extension List: ComponentContainer where Items == ComponentGroup { | |
} | ||
} | ||
|
||
// Component used to render a `<picture>` element for responsive image support. | ||
public struct Picture: Component { | ||
|
||
/// 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] = []) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, here's what I think we should do:
|
||
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: Plot.Image, source: Source) { | ||
JohnSundell marked this conversation as resolved.
Show resolved
Hide resolved
|
||
self.init(image: image, sources: [source]) | ||
} | ||
|
||
@ComponentBuilder | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick, but this attribute is not needed here, since a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean the |
||
public var body: Component { | ||
// if let sources = sources { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove all this commented-out code, please (and you can also remove the comment within the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, thought I had done that already. :-) |
||
Node.picture( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
// This creates a source that has null strings for those strings that have `nil` values, and `.empty` nodes for those `Integer`s with nil values. | ||
// Null strings are not rendered, so nothing is generated for them. | ||
|
||
.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) | ||
) | ||
} | ||
// else { | ||
// Node.picture(.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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This struct needs an explicit |
||
/// 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. | ||
public struct SubmitButton: Component { | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.