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

weseze created an issue. See original summary.

PA robot’s picture

Issue summary: View changes
Status: Needs review » Needs work

There 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.

dilipsingh02’s picture

Automated review :
Please fixed following issues :

EDIT: removed long pareview.sh dump.

klausi’s picture

@dilipsingh02: no need to post the long pareview.sh dump here, please attach it as txt file instead or link to http://pareview.sh

dilipsingh02’s picture

Hi @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

weseze’s picture

I 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.

weseze’s picture

Status: Needs work » Needs review
gisle’s picture

@weseze, is LazyLoad (lazyload.min.js) own work or something authored by a third party?

weseze’s picture

@gisle: third party library. I'm guessing I should include it as a library rather then included within the repo?

gisle’s picture

third party library. I'm guessing I should include it as a library rather then included within the repo?

Yes.

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.

weseze’s picture

@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

panshulk’s picture

Status: Needs review » Needs work

Hi @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 :)

weseze’s picture

Status: Needs work » Needs review

@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.

ikit-claw’s picture

Hi 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.

ikit-claw’s picture

Status: Needs review » Needs work
weseze’s picture

Status: Needs work » Needs review

Thanks @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

weseze’s picture

I'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.

John Pitcairn’s picture

Status: Needs review » Needs work

This 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

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
No: Does not follow the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
  1. (+) The DrImageController class should use a dependency injection container, not make direct calls to Drupal::service() and Drupal::moduleHandler()
  2. (+) hook_help() should be implemented to provide a Drupal help page, see the guidelines.
  3. file_stream_wrapper_valid_scheme() is deprecated, use \Drupal\Core\File\FileSystem::validScheme()
  4. DrIMageController::image() is rather long. Could the section that copies and slightly modifies ImageStyleDownloadController::deliver() be contained in a separate deliver() 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.

John Pitcairn’s picture

Status: Needs work » Reviewed & tested by the community

Actually, 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.

John Pitcairn’s picture

Followup issue for dependency injection: #2838505: Use dependency injection in DrimageController class

weseze’s picture

The 2 important issues have both been fixed.
I will address the other issues in the coming days/weeks.

apaderno’s picture

Assigned: Unassigned » apaderno
Status: Reviewed & tested by the community » Fixed

Thank 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.

klausi’s picture

Assigning credits.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.