-
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?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?
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.
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.
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:
- 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 singlesource
within 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
|
||
@ComponentBuilder | ||
public var body: Component { | ||
// if let sources = sources { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
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 singlesource
within 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.
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).
Added a Picture Component for use with the
picture
node.