Dynamic Responsive Images (or drimage) is a replacement module for the Responsive Image Style module (Drupal core). It is meant to take the pain away of configuring and maintaining responsive image styles by simply not needing any configuration.
Install it and configure your image field to display with the "Dynamic Responsive Image" formatter.
It differs from:
core image/image_styles: those are not responsive t all
core responsive images: require a lot of configuration and the configuration needs to be done by hand for every little change in design. Also does not work well for fluid layouts
adaptive_image: requires configuration, uses cookies to do its magic (can be a problem from reverse-proxy services), the cookie also prevents reloading of images when screen size changes within the same page. (it is not very dynamic)
Link to sandbox project: https://www.drupal.org/sandbox/weseze/2737687
GIT clone: git clone --branch 8.x-1.x https://git.drupal.org/sandbox/weseze/2737687.git drimage
cd drimage
Comments
Comment #2
PA robot CreditAttribution: PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpsgitdrupalorgsandboxweseze2737687git
Fixed the git clone URL in the issue summary for non-maintainer users.
We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #3
dilipsingh02 CreditAttribution: dilipsingh02 commentedAutomated review :
Please fixed following issues :
EDIT: removed long pareview.sh dump.
Comment #4
klausi@dilipsingh02: no need to post the long pareview.sh dump here, please attach it as txt file instead or link to http://pareview.sh
Comment #5
dilipsingh02 CreditAttribution: dilipsingh02 commentedHi @klausi,
Thanks for your message. I have attached the file for same.
@weseze
Automated review :
please review following URL:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxweseze2737687git
Thanks
Comment #6
weseze CreditAttribution: weseze commentedI have fixed almost all issues found. A few remain however and I am not able to fix them:
1) the included lazyload file is minified so a few things are considered wrong with it. I can not fix that, it is the nature of a minified file.
2) the custom JS code (drimage.js) is missing documentation. I have not included any because the code is so trivial and small.
3) the drimage.module is reported to be missing comments, but they have all been added. So am not sure what the problem is.
4) both the controller and formatter classes have code style issues, but the code is based on (and partially copied from) D8 core code. If it's not an issue for Drupal core code, it surely can't be for this module.
5) the custom JS code in drimage.js has to use !== and === but that actually breaks the code because the whole point of those 2 pieces of code is to check if things are the same but different in type.
Comment #7
weseze CreditAttribution: weseze commentedComment #8
gisle@weseze, is LazyLoad (lazyload.min.js) own work or something authored by a third party?
Comment #9
weseze CreditAttribution: weseze commented@gisle: third party library. I'm guessing I should include it as a library rather then included within the repo?
Comment #10
gisleYes.
As you can see under § 4.2 in the Project application checklist, the Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
Comment #11
weseze CreditAttribution: weseze commented@gisle: I don't really want to rely on unstable code...
So to try and fix this quickly I have removed the lazyloader from my module. It is not needed to make the core of this module work, so removing it seemed like the best approach to me. I did however create a task to add this functionality back in once the libraries API is stable in D8: https://www.drupal.org/node/2743639
Comment #12
panshulk CreditAttribution: panshulk commentedHi @weseze
Automated Review :
There are still few errors and warning related coding standards and Js Coding Standard reported by PAreview.sh . Please fix the error reported :)
Comment #13
weseze CreditAttribution: weseze commented@panshulk: I already explained in #6 under 5) why those warnings are not relevant:
"the custom JS code in drimage.js has to use !== and ===": that actually breaks the code because the whole point of those 2 pieces of code is to check if things are the same but different in type.
Comment #14
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedHi weseze you can work around the drimage.js by putting it on github and then pulling it in as an external that would get around the warning for you.
Put it on github here is an example of what you will need to add to your libraries.yml file
drimage.js:
js:
https://pathtofileongithub/bootstrap/3.3.7/js/drimage.js { type: external }
Make sure you have also listed it in the used libraries as well.
Comment #15
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedComment #16
weseze CreditAttribution: weseze commentedThanks @ikit-claw for the suggestion.
But I was able to work around the issue by using the isNan() function to rely on my type checking. This makes the code cleaner and easier to understand.
There are now no more errors: http://pareview.sh/pareview/httpsgitdrupalorgsandboxweseze2737687git
Comment #17
weseze CreditAttribution: weseze commentedI've made some minor tweaks and added support for focal_point module.
Can someone please help me ge this project promoted to a full project? This module has been running very reliably on 4 projects now and my colleagues are thrilled about it.
Comment #18
John Pitcairn CreditAttribution: John Pitcairn commentedThis is pretty nice, I will be using it, thanks. Found a couple of things that should be fixed.
Automated Review
Some best practice issues identified by pareview.sh / drupalcs / coder. See https://pareview.sh/node/491
Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.
Manual Review
hook_help()
should be implemented to provide a Drupal help page, see the guidelines.file_stream_wrapper_valid_scheme()
is deprecated, use\Drupal\Core\File\FileSystem::validScheme()
DrIMageController::image()
is rather long. Could the section that copies and slightly modifiesImageStyleDownloadController::deliver()
be contained in a separatedeliver()
method for readability and maintainability?The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #19
John Pitcairn CreditAttribution: John Pitcairn commentedActually, it is unclear to me whether not using dependency injection in a class or not implementing hook_help() should block RTBC status. On the basis that the the road to full project status is still too restrictive and prone to bikeshedding on things that could easily be handled in the issue tracker, I revise my previous opinion and mark this RTBC.
Comment #20
John Pitcairn CreditAttribution: John Pitcairn commentedFollowup issue for dependency injection: #2838505: Use dependency injection in DrimageController class
Comment #21
weseze CreditAttribution: weseze commentedThe 2 important issues have both been fixed.
I will address the other issues in the coming days/weeks.
Comment #22
apadernoThank you for your contribution!
I will update your account so you can opt into security advisory coverage. See Goodbye Project Applications, Hello Security Advisory Opt-in to understand what that means, and what is different from the previous project application purpose.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!
Thank you, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
Thanks go the dedicated reviewer(s) as well.
Comment #23
klausiAssigning credits.