Picture component #91
Conversation
It creates a set of `.sources` using all the supplied `Attribute`s relying the behavior of `Attribute` to not render null strings and/or `.empty` nodes. I tried to use the `.if` syntax, but got a compiler error. I am leaving the commented code in case there is a better way to implement this.
There was a problem hiding this comment.
Sorry for taking so long to review this @majortom64! I left a few comments inline, let me know if you want me to clarify any of them, and if I can assist you in any other way in getting this merged 👍
JohnSundell
added
the
awaiting update
Changed Plot.Image to Image (this was developed in a separate repo and then pulled into this fork, it was needed there, but is not needed here). Replaced two ternary ops with `.unwrap` in the `Picture` `body`.
|
@JohnSundell Thanks for getting to this. Mine was not waiting that long, so I am glad they got done so quickly. I will add tests for the node changes I made, so that can get merged as well. |
There was a problem hiding this comment.
Thanks for updating your PR, @majortom64! Just did another review pass on it - it's looking good, but there are some additional changes needed to make the code fully mergable. Let me know if you need any additional clarification or assistance from my side, and please do implement unit tests covering these additions 👍
| 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.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So, here's what I think we should do:
- Make this initializer (the one that takes an array of
sources)public, since it covers the majority use case. - Remove the initializer that takes a single
source(since that same functionality can easily be achieved by simply passing a singlesourcewithin an array). - Add parameter documentation to the initializer (similar to what the other built-in components have).
| self.init(image: image, sources: [source]) | ||
| } | ||
|
|
||
| @ComponentBuilder |
There was a problem hiding this comment.
Nitpick, but this attribute is not needed here, since a single Component is returned from the body property.
There was a problem hiding this comment.
You mean the @ComponentBuilder? Happy to remove it, still do not really understand all DSL intricacies.
| @@ -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 { | |||
|
|
|||
There was a problem hiding this comment.
Code style nitpick: Please remove this blank line to make this type definition consistent with the rest of the library.
|
|
||
| @ComponentBuilder | ||
| public var body: Component { | ||
| // if let sources = sources { |
There was a problem hiding this comment.
Remove all this commented-out code, please (and you can also remove the comment within the forEach as well, I think, as the code is quite self-explanatory).
There was a problem hiding this comment.
Sorry, thought I had done that already. :-)
Changed one final `Plot.Image` to `Image` Cleaned up some formatting and black lines. :-)
|
I think I have done all the things you asked (other than removing the initializer for which I explained why I think it is still needed). Is there anything left outstanding? I am going to go work on tests for my Attributes PR, if this one is basically done. Let me know if I still have anything outstanding. |
| } | ||
|
|
||
| public var body: Component { | ||
| Node.picture( |
There was a problem hiding this comment.
Sill incorrect indentation here. Please format your code according to the rest of the library (4 spaces indentation per level) to keep things consistent.
| 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.
So, here's what I think we should do:
- Make this initializer (the one that takes an array of
sources)public, since it covers the majority use case. - Remove the initializer that takes a single
source(since that same functionality can easily be achieved by simply passing a singlesourcewithin an array). - Add parameter documentation to the initializer (similar to what the other built-in components have).
|
|
||
| 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.
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).
Added a Picture Component for use with the
picturenode.