Problem/Motivation
Right now the Responsive Image has a number of key Developer Experience flaws. Many of these can be solved by converting the theme functions to a render element #type, with a few other tweaks.
By making these changes, responsive images should be easier to use both in the field formatter context as well as when responsive images need to be used when a formatter is not available.
Using #type allows us to attach the Picturefill library, rather than requiring themes to add that manually in a preprocess function.
Another key DX improvement is being taken care of in #2349461: Move fallback image style into the responsive image style entity. Once that lands, we can remove some fallback code in theme_responsive_image_formatter that deals with a mapping ID not being provided.
For responsive_image_formatter, we should change #url to something like #link_path to make clear that this is the URL for a link, not the URL of the original image.
For responsive_image, we need to clarify the correct format for #uri, as right now that can only accept a stream-wrapped URL like public://path/image.jpg
Taking care of this may eliminate the need for #1898442: responsive_image.module - Convert theme_ functions to Twig.
If there's a way for us to use the same #type for both responsive_image and responsive_image_formatter, and the formatter just makes use of the same #type that you would use in a preprocess function, that might make this a lot easier to work with.
Proposed resolution
Improve the DX of this significantly.
Remaining tasks
TBD.
User interface changes
None.
API changes
TBD.
Related Issues
#2061377: [drupalImage] Optionally apply image style to images uploaded in CKEditor 5
Original Issue Report
Picture.module has been developed with only one use case in mind: field formatters. And it does that well.
However, it's very plausible and perfectly reasonable for developers to want to output images besides image fields using <picture>
as well. Once you want do that, you need to look at how PictureFormatter
works:
PictureFormatter::viewElements()
is the entry point of the formatter. There's first 30 LoC just to build the#breakpoints
property thattheme_picture_formatter()
expects. This belongs in a helper function. In fact, it probably should be something likePictureMappingInterface::getMapping()
.- Then there's this:
$elements[$delta] = array( '#theme' => 'picture_formatter', '#attached' => array('library' => array( array('picture', 'picturefill'), )), '#item' => $item->getValue(TRUE), '#image_style' => $fallback_image_style, '#breakpoints' => $breakpoint_styles, '#path' => isset($uri) ? $uri : '', );
- It would be much better if anybody who wants to output a
<picture>
can just'#type' => 'picture'
, which would allow you to set#attached
automatically —#theme
does not allow for that. #image_style
is a misnomer, it should be called#fallback_image_style
.#breakpoints
is probably a misnomer too, shouldn't it be called#mappings
?- In fact, why do we even have to set
#(fallback_)image_style
and#breakpoints
— why not just set#mapping_id
? The answer to this is probably "because theme functions don't allow for that", to which my answer is "then why don't we just use#type
instead?". #path
should probably be renamed to#link_path
, otherwise it might be thought to be the image path.
- It would be much better if anybody who wants to output a
- Then, if we look at
theme_picture_formatter()
, we see that it branches off into eithertheme_image_formatter()
(if#breakpoints
is not set, which I don't yet understand how that could happen and especially why we should support that) ortheme_picture()
. '#theme' => 'picture'
(i.e.theme_picture()
) is what I want to use, so that means here too I have to#attach
thepicturefill
library. Furthermore,theme_picture()
— liketheme_picture_formatter()
— accepts#breakpoints
: this means I have to duplicate those 30 LoC. Finally,#image_style
is now suddenly called#style_name
(and is undocumented).theme_picture()
's docs for#uri
are off/outdated: they claim "uri: Either the path of the image file (relative to base_path()) or a full URL", but in fact it only works with URIs likepublic://foo.jpg
.- Impossible to set custom attributes on the
<picture>
tag, due to this code:
$attributes = array(); foreach (array('alt', 'title') as $key) { if (isset($variables[$key])) { $attributes[$key] = $variables[$key]; } } $output[] = '<picture' . new Attribute($attributes) . '>';
I need to be able to set custom attributes to make this e.g. work with Drupal core's
FilterCaption
'sdata-caption
attribute.
Beta phase evaluation
Issue category | Task |
---|---|
Issue priority | Major because it highly increases the DX for themers and developers using responsive images. |
Prioritized changes | The main goal of this issue is improving developer experience. This is also further cleanup after the larger cleanup in a recent critical change: #2260061: Responsive image module does not support sizes/picture polyfill 2.2 and is also related to #2349461: Move fallback image style into the responsive image style entity. |
Disruption |
May be disruptive to a small number of beta sites. Any effects on core systems are addressed in this patch, and there are no known contributed modules this would affect. As of April 20, 2015, there are roughly 250 public beta sites. Responsive images module is not enabled by default, so a smaller number of those sites could be using responsive images. Even a smaller number of those site use custom code (meaning not using the field formatter) to output responsive images. These sites would have to update their custom code or any contrib modules using custom code to output responsive images. The benefits of this change outweigh the disruption, as it allows for developers to output a responsive image with just a few lines of code, which is a big simplification for the much larger number of sites that may use this in the future. |
Comment | File | Size | Author |
---|---|---|---|
#107 | improve_responsive_img_dx-2123251-107.patch | 11.17 KB | attiks |
#107 | interdiff.txt | 693 bytes | attiks |
#98 | improve_responsive_img_dx-2123251-98.patch | 11.43 KB | Jelle_S |
#18 | drupal8.picture.2123251-18.patch | 18.57 KB | marcvangend |
#11 | drupal8.picture.2123251-11.patch | 17.75 KB | Sutharsan |
Comments
Comment #1
Wim Leers.
Comment #2
Wim LeersAdded point 6.
Comment #3
attiks CreditAttribution: attiks commented@wim thanks for the extensive explanation
The easy ones
3/ This is ported like that from D7 version, and was used as a fallback mechanism, but you're right it should be removed
5/ Also a remain of the D7 version, not anymore needed in D8, comment needs updating
6/ Known issue should be fixed and backported to D7 version
This leaves 1, 2 and 4. If I understand correctly we better use '#type' => 'picture' and instead of passing breakpoints around we need to pass the mapping_id?
Comment #4
Wim LeersI personally think that would be the best DX. But please don't just listen to me. Let others chime in. I've pinged nod_.
Comment #4.0
Wim LeersAdded point 6: inability to set custom attributes on <picture>.
Comment #5
tim.plunkettNew d.o is stripping that.
Comment #6
Jelle_SFirst stab at a patch following #0. Should fix all points made there.
Comment #8
Jelle_SFails seem unrelated to this patch...
Comment #9
Jelle_S6: drupal8.picture.2123251-6.patch queued for re-testing.
Comment #10
RainbowArrayPatch no longer applies.
Comment #11
Sutharsan CreditAttribution: Sutharsan commentedPatch #6 rerolled.
Comment #12
attiks CreditAttribution: attiks commentedFYI: picture is renamed to responsive_image so this needs a reroll once #2124377-74: Rename "Picture" module to "Responsive Image" module is committed
Comment #13
star-szrLess yelly title :)
Comment #14
nod_less mangled title.
Comment #15
star-szrOh yeah, D7 d.o--
Comment #16
star-szrAdding a very related issue that overlaps/maybe clashes with this. This will also need a reroll as mentioned by @attiks.
Comment #17
Eli-TComment #18
marcvangendReroll of #11. I didn't test it yet (I will later today or tomorrow) but at least it applies cleanly.
There's one thing I wasn't sure about: #2211831 removed the alt and title attributes from the picture element in theme_responsive_image(), while this patch uses
$output[] = '<picture' . new Attribute($variables['attributes']) . '>';
which implicitly allows alt and title attributes again.Comment #19
marcvangendComment #20
marcvangendPatch in 18 contains an error, this one is better.
Comment #21
Wim LeersThank you so much for working on this! :)
Here's an initial review. Solid progress, but I feel more helper methods might be able to help make the code much more legible still. There are still many levels of nesting: nested for-loops in if-statements are frequently present. Helper methods can help simplify that.
Much clearer! :)
Also much clearer :)
Why mention the "ID" part? Why not just
#mapping
?This looks bizarre, but it's probably necessary to pass
TRUE
?Missing
@return
documentation.This probably needs to be uncommented again :)
s/The uri/The URI/
s/as fallback/as the fallback/
There's a subtle but important difference between the old and the new code here.
The retrieved breakpoints contains a mapping of breakpoint names to multipliers.
The multipliers, in turn, contain a mapping from multiplier to image style.
That doesn't mean they're the *fallback* image style. It just means they're the image style associated with that breakpoint at that multiplier! So AFAICT the new code introduces a bug.
This one should also be a property, not a child.
Where is the URI of a new source being set?
This seems wrong. AFAIK this is meant to allow making the
<picture>
clickable, to link to e.g. the original image.But this
return
prevents the<picture>
element from being returned, instead returning a link?Comment #23
Wim LeersOn explaining my feedback in #21, we noticed that point 9 is probably wrong and point 12 is completely wrong!
Comment #25
RainbowArrayIn response to #18, alt and title should not be output on the picture element.
This issue is also relevant to solving that: #2220865: Add empty img element inside picture element.
Comment #26
marcvangendRe #25: I see. Currently, there is just one attributes array, which is applied to the picture element as well as the img element. The question remains we should be able to add other attributes to the picture element. I think we should - there's no way we can be sure it will never be needed. I guess there are two options: 1) add the attributes to the picture element except 'title' and 'alt', or 2) provide a separate attributes array for the picture element.
Comment #27
marcvangendI did a simple experiment just to see if the patch in #20 makes it easier to leverage the responsive_image module from custom code. I'm attaching the example module, "custom_respimg", to this comment. The important part is the render array in the page controller:
This code assumes that:
- there is a file called "ddd-logo-l.png" (file included in the tar.gz) in the public:// files directory;
- there is an image style called "large";
- that there is a responsive image mapping called "default".
If everything is OK, the module should display a responsive image at /custom_responsive_image.
Comment #28
marcvangendFixing a stupid mistake, tests should pass now.
Comment #30
marcvangendSo, my patch no longer applies because in the mean time [#8611447-36] was committed. That patch defines a ResponsiveImageMappingInterface::getMappings() method which makes rerolling this patch more difficult.
It may be wise to postpone this issue on #2219329: ResponsiveImageMapping::mappings and ResponsiveImageMapping::breakpointGroup properties have inconsistent return structure and type respectively, which aims to make the format of breakpoint groups and responsive image mappings more consistent.
Comment #31
attiks CreditAttribution: attiks commented#2260061: Responsive image module does not support sizes/picture polyfill 2.2 contains a bit rewrite, so it might be better to merge the remaining parts into that issue, or postpone it till that is committed
Comment #32
Wim Leers#2260061: Responsive image module does not support sizes/picture polyfill 2.2 got committed (YAY!), hence this is now unblocked :)
Comment #33
attiks CreditAttribution: attiks commented#32 I think we need to update the IS, because most of it is no longer relevant
Comment #34
RainbowArraySince the responsive image module code has been completely overhauled, it's highly like that much of this issue isn't relevant anymore. But somebody should take a look to see if any of this is still relevant.
Comment #35
Jelle_SFrom the issue summary:
1. is mute because of the points made in 2. Also there is now
ResponsiveImageStyle::getImageStyleMappings()
andResponsiveImageStyle::getKeyedImageStyleMappings()
2. We now use
#responsive_image_style_id
and no longer use#breakpoints
. All other points are still valid. Except we use#url
in stead of#path
(which might still not be ideal?).3. & 4. These points are also still pretty valid. Except we switched
theme_picture()
totemplate_preprocess_responsive_image()
andresponsive-image.html.twig
.5. Docs are ok now, but naming isn't (see 2. in this comment)
6. I believe there is a separate issue for this. Let me track it down... Jup here it is: #2421317: Do #attributes/$variables['attributes'] to go the <picture> or the fallback <img>?
Comment #36
RainbowArrayTrying to put together a to-do list, as it's pretty complicated right now to understand what needs to be done.
Non-field-formatter use cases
The big issue is how can responsive images be used outside of a field formatter context. Field formatters are great, but the last D7 project I worked on had numerous situations where a field formatter wasn't available. I ended up using the theme_picture function but had to feed in the fallback style and breakpoints: it was really awkward, and I'm hoping this can be done better in D8. We may or may not be able to make those improvements at this point, but we can try.
Set fallback style in mapping
If we can avoid passing in the fallback style, and let the mapping take care of that, we'd be better off. #2349461: Move fallback image style into the responsive image style entity does just that.
#type
By changing this to a #type, the Picturefill library could be attached automatically. Preventing people from setting that manually would be a big win.
Change #path to #link_path
Make it clearer that this is for when the image should link to something.
Fallback for no responsive mapping ID unnecessary?
Particularly if we move the fallback style into the mapping, then I have a hard time seeing why a formatter would be set without a mapping being set, where we'd need to fall back to a simple image. Can that fallback be removed?
Clarify URI format in docs and variable name
I definitely ran into confusion over this. If only a stream-wrapped URI is an acceptable value, that should be clear, and the variable name should reflect that.
Picture element attributes
There is very brief discussion about that in #2421317: Do #attributes/$variables['attributes'] to go the <picture> or the fallback <img>?. The global attributes are allowed on picture, although in almost all cases it is probably better to set things like class on the controlling img inside picture. Anyhow, here are the global attributes: https://html.spec.whatwg.org/multipage/dom.html#global-attributes The challenge is how to single out what attributes should be for picture vs img.
So here are the big issues that look like they still need to be solved that aren't being handled elsewhere:
Four and five are pretty straightforward. The first three items may need discussion and decisions. We may want to split some of these out to separate issues.
Thoughts?
Comment #37
RainbowArrayOh, also, #1898442: responsive_image.module - Convert theme_ functions to Twig will convert theme_responsive_image_formatter to template_preprocess_responsive_image_formatter. There may or may not be overlap with this issue.
Comment #38
Jelle_SMy 2 cents:
#36.1: If we do #36.2, this wil become way easier.
#36.2: I think we should, would be a huge DX win.
#36.3: Yes, I would remove it, I don't see any use for it anymore.
#36.4: ++
#36.5: ++
Comment #39
Wim Leers+1 to the general analysis in #36. Thank you so much for posting this and getting this going again :)
Comment #40
RainbowArrayComment #41
attiks CreditAttribution: attiks commentedSince #2349461: Move fallback image style into the responsive image style entity is green, let's wait till it gets committed.
I'll ask Jelle if he feels like working on this
Comment #42
Jelle_SI feel like it ;-)
Comment #43
marcvangendGreat. I'll be happy to test your patch and see if it improves my X as a D :-)
Comment #44
Jelle_SHere's the new patch. Since A LOT has changed to Responsive Image since the last patch an interdiff isn't of any use here.
This is a patch on top of #2349461-27: Move fallback image style into the responsive image style entity (hence the do-not-test), since that patch makes writing this patch a bit easier and will probably soon get committed anyway.
All relevant tests were green on my machine, but I couldn't run all tests.
So to recap from #37:
'#theme' => 'responsive_image'
has been changed to'#type' => 'responsive_image'
'#type' => 'responsive_image'
directly, so the conversion to twig is also no longer necessary.Comment #45
Jelle_SComment #46
mondrake1.
hook_element_info() is replaced by annotated classes, see https://www.drupal.org/node/2320115
also you can attach libraries in twig templates directly now, see https://www.drupal.org/node/2456753
2.
and consequently
shouldn't this be
?
Comment #47
Wim LeersHah! :D
#44 <3 <3 <3
+1 to #45.1, too tired for #45.2 now. But NW for #45.1 at least.
Comment #48
marcvangendI'm working on this at the Drupal Dev Days.
Comment #49
marcvangendPatch attached should take care of #46.1. I ran all simpletests in the responsive image group locally, they all were green.
Comment #50
marcvangendI updated my test module which shows that this actually works. The DX has indeed improved a lot; it's pretty easy now to output a responsive image element. Of course a responsive image style must be defined, but it was pretty easy to define it, export it to a yml file and include it in the module.
If you want to test it yourself:
.tar_.gz
extension first)ddd-logo.png
in your files folderI couldn't figure out if it's possible to serve the image from the module's directory (or from an external URL) instead of the files directory. Being able to do that would make it even more flexible for custom code.
Comment #51
marcvangendBy the way I don't think it's necessary to convert the theme hook from 'variables' to 'render element'. After all the information passed to the theme layer is not a render array, but just a bunch of variables that needs to be processed in
template_preprocess_responsive_image()
.Comment #52
Wim Leers#51 is correct. NW just for that.
Comment #53
marcvangend@Wim did you mean to refer to #50? Because #51 only says we do not have to change the hook_theme implementation, so there's no work needed there.
Comment #54
Wim LeersOops! You're absolutely right :)
So, here's a review. This looks like it's almost RTBC. Just nitpicks.
Missing
{@inheritdoc}
docblock.s/array()/[]/
Why? If cosmetic, no further doc changes are necessary. If necessary for responsive images to work as expected, then this should get a comment.
Comment #55
Jelle_SRE #52: So we need to be able to serve the image from the module's directory (or from an external URL) instead of the files directory?
Module's directory will probably work (given that the file has the right permissions), but external URL is difficult I'm afraid, since we need to know the dimensions of the given image (unless D8 Core can handle that?)
I'll start work on the points made in #54.
Comment #56
Jelle_SNew patch with points made in #54.
Comment #57
marcvangend@Jelle It would be very nice if images can also be loaded from the module directory or from an external URL. I think I've read somewhere that regular image styles in D8 would support external images too, but I couldn't find that post so maybe I am mistaken.
If we can add support for module-directory files (and possibly external files) to this patch with not too much effort, let's do it. If it would take hundreds of lines of code, and several weeks to review and test, then I propose to get this patch committed now and create a follow-up issue.
Comment #58
Jelle_SAccording to the current documentation of template_preprocess_image_style image styles do not support anything other than stream wrappers (URIs), meaning we don't either since responsive image styles use image styles to output the
<source>
attributes. So IMHO this should be fixed/added to image styles and my best guess is (with a bit of luck, but I'm not sure) we'll support it out of the box when or if that happens.EDIT:
There's #1308152: Add stream wrappers to access extension files as well, which would probably help a lot.
EDIT2:
#1358896: Flexible scheme and URI for image derivatives is another related one.
Comment #59
Jelle_SRe-uploaded patch without do-not-test since #2349461: Move fallback image style into the responsive image style entity got committed (woohoo!).
Comment #61
Wim LeersIs it the module, or the template?
Wouldn't it be better to update the template to not output whitespace?
Comment #62
Jelle_SYeah sorry it's the template. We could do that, but in my personal taste the template looks better / easier to read with the whitespace:
vs
Comment #63
Wim LeersAgreed regarding legibility.
But: http://twig.sensiolabs.org/doc/templates.html#whitespace-control
Comment #64
Jelle_SOh nice! I'll change it then.
Comment #65
Jelle_SNew patch, added
{% spaceless %}
Comment #66
Jelle_SForgot to update one test with the new missing whitespaces.
Comment #68
marcvangendRegarding external/module files: I think #1308152: Add stream wrappers to access extension files looks like a very nice solution, much better and more generic than what we could achieve in this issue. Let's leave that for now.
Thanks Wim for the
{% spaceless %}
tip, I didn't know that one yet.I'll test the new patch in combination with my proof-of-concept module asap.
Comment #69
joelpittetFYI, if you still see whitepsace that you don't want use the other whitespace modifiers.
{{- -}}
or{%- -%}
becausespaceless
doesn't like to go into other control structures deep.Here's what I'm babbling about:
http://twigfiddle.com/gcenh6
Comment #70
RainbowArrayIt looks to me like the responsive image formatter is doing some extra things to extract the image's height and width that isn't being done if somebody is using responsive images outside of a field formatter. It would be great if it were just as easy to use responsive images outside of a field formatter, so if that height and width is necessary, can that be done somewhere that benefits both use cases?
Comment #71
RainbowArraySetting to needs work to address my questions about height and width, as well Joel's suggestions for how to handle whitespace.
Comment #72
joelpittetNot a suggestion more an FYI for when you ask "why you no work like I expect spacesless!"
Comment #73
Jelle_SRE #70: In
responsive_image_build_source_attributes
we default to the image width and height if they aren't given, which is basically what the formatter does as well. Since this is double code and might confuse some people (they will probably look at the formatter for an example on how to output a<picture>
tag). I think removing this part of the code is within the scope of this issue as it improves DX of responsive images, so I did. Relevant tests were still green on my local machine.Comment #74
Jelle_SOops, didn't remove enough code :-).
Comment #75
attiks CreditAttribution: attiks commentedThis looks great, thanks all for working on this.
Comment #76
webchickLet's get a beta evaluation for this please in the issue summary, since it's changing APIs but not fixing a critical issue.
Comment #77
Jelle_SComment #78
Jelle_SI added the beta evaluation to the issue summary.
Comment #79
attiks CreditAttribution: attiks commentedIS update looks good, does it also need a CR?
Comment #80
RainbowArrayYes, need a CR since this changes how to work with responsive images outside of field formatters.
Comment #81
attiks CreditAttribution: attiks commentedDone: https://www.drupal.org/node/2475903
Comment #82
joelpittetVery nice, had another look at the patch and it looks like a nice DX improvement to not have to deal with the library attachment!
Comment #83
marcvangendMy €0.02 regarding the beta change evaluation:
IMHO this is an API addition rather than an API change. At this moment, there is hardly anything that I would call an API if you want to render a responsive image in custom code. Maybe it's currently possible to trick Drupal into thinking that you're rendering an actual field, but that's not an API, that's a hack.
Even if this is considered an API change (admitted: the theme hooks have changed) then I'm sure that impact > disturbance. Looking at the definition of a disruptive change, I think none of the 6 points apply in this case.
Comment #84
joelpittetWhoa that's like $0.03 CAD:P I agree, any chance you can massage those points a bit to your perspective?
Comment #85
Dave ReidThe new render element has me confused. I guess I expected basically a parity with the existing image.html.twig element and what it supports, so that with minimal changes I could change from '#type' => 'image' to '#type' => 'response_image'. Why does this support linking to something when the output should be wrapped in a separate link element itself? I'm used to theme_image vs theme_image_formatter, I guess I expected the same, and now having two different ways of outputting a basic image vs responsive image, seems like worse DX in my mind.
Comment #86
joelpittetOh that's a great point @Dave Reid, going to bump it back for some discussion here.
Maybe we can extend #type=>image to get some parity/consistency? (Element class extending kinda/sorta works but pain to merge callbacks), there are examples with some of the form element's extending base elements.
Comment #87
attiks CreditAttribution: attiks commented#85 If I understand you correctly, you propose to provide two twig templates:
- image-formatter.html.twig
- image.html.twig
And keep all variables as close as possible?
Comment #88
Wim Leers#87: yes.
And, indeed, great feedback Dave Reid! :)
Comment #89
Jelle_Sok, so
#type => responsive_image_formatter
or#theme => responsive_image_formatter
(second one I guess?)Comment #90
attiks CreditAttribution: attiks commentedI closed #1898442: responsive_image.module - Convert theme_ functions to Twig since it will be part of this patch, so we can commit both twig templates at the same time.
Comment #91
marcvangendre #89, I think there is no need to turn the formatter itself into a render element, so that would be
#theme
, not#type
.But... How would image-formatter.html.twig be different from responsive-image-formatter.html.twig? I mean, if the responsive image module provides a render element and a field formatter, couldn't the field formatter simply use
'#theme' => 'image_formatter'
to wrap the responsive image element in a link?Comment #92
RainbowArrayMy understanding is that the responsive_image #type is more akin to an image style than to an image. Does that help, @dave-reid?
I'm also unclear why we're talking about going back from #type to #theme. My understanding is we switched to #type in order to be able to attach the Picturefill library.
Comment #93
marcvangend@mdrummond, we're not talking about going back from #type to #theme. We keep
'#type' => 'responsive_image'
, but we should bring back'#theme' => 'responsive_image_formatter'
.In the current HEAD, there are two theme hooks, 'responsive_image' and 'responsive_image_formatter'. The patch in #44 (and all patches after that) merged the tasks of those two theme functions into a single render element. If we want a clear separation of concerns, then the responsive_image render element should not be responsible for wrapping the responsive image in a link. Wrapping it in a link should be the job of the formatter, because it's the formatter that provides a setting to link the image. Image module follows the same pattern: it separates
'#theme' => 'image'
(which is not a #type, because it doesn't need to add js by default) from'#theme' => 'image_formatter'
.Comment #94
RainbowArray@marcvangend After my last comment, I was looking at the image module to try to grok what was being proposed. I think I'm almost getting it, and your comment helped clarify further. Not entirely there yet, but then again, I have a bad cold.
As long as we're keeping the benefits of the #type conversion, makes sense to me to restrict the image link to the formatter, as those two don't need to be glommed together if you're calling this through preprocess.
Comment #95
Jelle_SMy local machine's tests were being weird. Checking if it's my local setup or a general problem with the code.
I tried to mimic the image formatter as much as possible so everything should be pretty similar. Let's see what the testbot thinks.
Comment #96
marcvangendThanks Jelle, it looks pretty much like what I expected. Some comments on the code, and one question:
I know Image module does this too, but can we break the array up in multiple lines please?
s/html/HTML/
s/An/A/
No criticism, but just for my understanding: why does all this processing happen in template_preprocess_responsive_image_formatter() and not in ResponsiveImageFormatter::viewElements() ? Can someone enlighten me?
Why did this change? Was the previous doc incorrect, or did we lose the ability to use a full URL somewhere?
This doesn't seem right. Given that image-formatter.html.twig says "Default theme implementation to display a formatted image field.", I guess this should be "Default theme implementation to display a formatted responsive image field". It's not the theme that has become responsive; it's the image.
Comment #97
Jelle_S#96.4: This is basically copy paste from the image module (aside from the obvious changes). I guess we could do all that in
ResponsiveImageFormatter::viewElements()
, but I thought it would be better being consistent with the image module.I'll address your other points.
EDIT:
#96.5: The doc was incorrect (see the issue summary).
Comment #98
Jelle_SNew patch, only comment & code style changes so should still be green.
Comment #99
marcvangendThanks Jelle.
Re #96.4: I understand that you have followed the structure of template_preprocess_image_formatter() and ImageFormatter::viewElements(), and I agree that that's the best approach for this patch. I'm just interested in the rationale behind this structure. If there is no logical explanation, we could create a follow-up issue to clean it up in both modules.
Comment #100
Jelle_S#99: Yeah, I don't understand the rationale behind it either. I don't know who wrote that code, but it would be interesting to hear from them about their reasoning behind it.
Comment #101
mondrake#99 and #100: no particular rationale for D8, just a minimal change from D7 code when converting to Twig. Twig conversion approach was always to minimise code changes, and let optimisation for follow ups.
Comment #102
marcvangendThanks for the info, @mondrake.
The patch in #98 looks really good to me. It solves the 5 points from #36 and addresses the concerns raised by Dave in #85. The tests are green, my proof-of-concept module shows that it works in real life... I see no reason why this wouldn't be RTBC.
Comment #103
RainbowArrayThis looks good to me as well. Greatly simplifies how responsive images are used.
I think if there are concerns that preprocess can be simplified, both for images and responsive images, then let's file a follow-up issue for that. The preprocess code doesn't seem like a deal-breaker to me, but if there's a better place to take care of those checks, sure.
Thanks for everybody's hard work on this. Feels like responsive images is getting into a much better place now. After this, only a couple really important issues that still need to get in for responsive images in D8 from my point of view.
Tweaking the UI to allow sizes to be configured is pretty important: #2334387: UI changes to support current responsive image standards.
Could also some review on source attribute streamlining: #2423743: streamline responsive_image_build_source_attributes().
Feel free to start taking a look at those while we wait for core committer reviews on this.
Comment #104
alexpottAm I correct in thinking that if a site has not done any custom theming of responsive image this change is not disruptive at all?
Comment #105
attiks CreditAttribution: attiks commentedTrue
Comment #106
alexpottNot used.
As to whether or not this is eligible for beta inclusion - I'm inclined to agree that this is followup to the recetn critical and bringing responsive image inline with the rest of core. The work to bring this in line with @Dave Reid's suggestion in #85 is encouraging.
Comment #107
attiks CreditAttribution: attiks commentedLine removed
Comment #108
RainbowArrayConcern in #106 resolved. Moving back to RTBC.
Comment #109
alexpottI'm committing this under the reduce fragility (image and responsive_image working the same makes lots of sense) and the followup provision made in the beta evaluation policy. I think the risks of making this change are extremely minor because responsive_image is not enabled by default and if a site had installed it they would have to have do some custom theming to be affected.
Committed 9451f8a and pushed to 8.0.x. Thanks!
Comment #111
RainbowArrayGreat to see this get in! Thanks alexpott!
I found a small omission that could use a follow-up #2478667: Remove link_path from responsive_image theme. I think we intended to remove link_path from responsive_image to better match image, but we missed that. One line change, so hopefully we can get that fixed quick. I updated the change notice at https://www.drupal.org/node/2475903 to reflect that link_path shouldn't be used.
Comment #112
star-szrCan we get that change notice updated to reference this issue please?
Comment #113
RainbowArrayLinked this issue and the follow-up in the change notice. Thanks for the suggestion, Cottser.