Hi,

Well, so this is my first attempt to get a module on d.o.

The module is a simple input filter, to be used in text formats. It builds columns out of content in body fields; automatically using jQuery Columnizer or manually by wrapping content in columns separated by breaks. A description of what it does can be found on the sandbox page:
https://drupal.org/sandbox/genox/2032173

  • I ran the source thru coder and coder tough love and implemented all suggested changes, except for a "$foo++ -> $++foo". Just doesn't feel natural to me since I used this for years and years. But I'll happily change it if you require me to do so.
  • I tried to write the help section and filter tips as good as possible, but I'm not a native english speaker, so .. :-)

Thank you!

Comments

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxgenox2032173git

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.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

I cleaned up the reported errors by the PA robot and created a "7.x-1.x" branch (removed "master" branch).

  • The CSS is generated by SASS/compass. It's output is never gonna match the desired formatting without manual edits, which kind of nullifies the idea of a preprocessor. However, I don't see how that's a problem. Original .scss file and a config.rb is supplied with the module.
  • The JS is a) minified and b) the non-minified library is not in my control to reformat to follow d.o. coding standards. The module's code validates.
  • The library (jQuery Columnizer) which is provided with this module is released under CC attribution 3.0 which - as far as my understanding is - should easily allow redistribution as part of a GPL project as long as the author is credited, which is visible a) in the source file and b) on the project's page. Do I handle this correctly? I don't see the point of adding a dependency for the Libraries module, since the module itself is very small and the library itself as well. IMHO it makes things more complicated for the enduser.
jiong_ye’s picture

jiong_ye commented
Status: Needs review » Needs work

There are a few uninitialized variables within a switch statement.

  1. $columns_current line 130 column_layout.module
  2. $columns_total line 131 column_layout.module
  3. $columns line 132 column_layout.module

Have you consider making your module dependent on the shortcode module. https://drupal.org/project/shortcode
I have used it for company projects. It provides a nice API for handling and processing shortcodes. It frees you from having to handle more complicated attributes and allows you to focus on your column logics

Anonymous’s picture

Anonymous (not verified) commented

Thanks for having a look. I set error_reporting(-1) and I don't get any reports about uninitialized variables. Can you check again? I adjusted a couple of things meanwhile.

I had a look at the module you mentioned. I would like to avoid introducing dependencies for this rather simple matter. I wouldn't have a need for any of the other shortcodes which are included in the module (yet). But I might reconsider when I add more functionality later on. For now, I would like to avoid dependencies.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

forgot to change status ... :-P

Anonymous’s picture

Anonymous (not verified) commented

By the way, here's an example of the current output: http://d7dev.genox.ch/test

asplamagnifique’s picture

asplamagnifique commented
Status: Needs review » Needs work

Hi

Some clean you can easy apply :

  • Install.txt l.5 remove '; files[] = column_layout.test'
  • column_layout.module :
    • line 71 : 1 space to remove on start of the line
    • line 72 to 76 > comments, must be remove in the final version of the module

String used on t() function seems clean.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

Thank you for the review. I committed the changes.

hatuhay’s picture

hatuhay commented
Status: Needs review » Reviewed & tested by the community

Hello @genox,
Nice work!!
Checking functionality.

  • Installation smooth.
  • Enabled following instructions OK.
  • Tested with jQuery update: 1.5, 1.7, 1.8, no problems.
  • I used CkEditor 4.1 enabled on standard Full HTML, but have to switch to plain text editor because CKEditor add code tags wrapping the brackets.
Anonymous’s picture

Anonymous (not verified) commented

Thanks for the review. It shouldn't make a difference wether the the shortcode tags are being encapsulated by HTML tags or not. The module runs the HTML code thru the HTML corrector provided by D7 in order to fix broken semantics (missing closing tags, missing opening tags etc.) after breaking the content into chunks and then removes the empty tags.

I just updated the version in the sandbox with finetuned Columnizer settings and a couple of fixes and optimizations on other ends. I use the module already on a couple of productive websites and it seems to works pretty good so far. It's a lot easier to get a couple of simple columns working here and there, without the need for complicated setups. However, I don't get around getting to test each and every edge case myself..

I would appreciate it if the module were "allowed" to be published in the d.o. catalog.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Assigned: Unassigned » klausi

I'll look at this now in the Project applications sprint

dman’s picture

dman commented
Assigned: klausi » Unassigned
Status: Reviewed & tested by the community » Needs work

Hi genox.

I'm afraid that the drupal.org policies explicitly forbid inclusion of third party code libraries :-( .
Not only, but CC-BY-3.0 is incompatible with GPL, which is required by all files uploaded for hosting on d.o. - as per the Terms of service everyone agrees to.
And, even if it were, Simply being GPL is still not enough for inclusion for distribution from here, because drupal.org doesn't host GPL-"compatible" code

That sort of puts a blocker on the module you are putting up for review here.

There are work-arounds - which generally involve removing the conflicting code and providing user instructions for fetching it themselves. It's a bit of a hassle, but it's all about respecting the licenses correctly. As the code review process is all about doing things in the legal way, you'll have to do this right.

Your README and help shows a good start though, and your structure is good.
I'd hope that a contrib module based on code of your own would have a better chance of passing review.

I don't see that adding the requirement of SASS just to use this module is appropriate here. It seems to add complexity and complicates integrating it with existing themes. Can this module not work without that?

I'm tempted to push this right back to 'closed' as the javascript license thing it a total roadblock, but if you think this can be useful without including the licensed code directly, then it could be worked on.

Anonymous’s picture

Anonymous (not verified) commented

To clarify, if I "outsource" the code as a library which has to be manually added, I get green light?

If so, I will add a dependency to the Libraries module and maybe even go the extra mile to create a drush command to fetch the current version and place it in the sites libraries on a user's command. Writing such a library myself is out of the question.

SASS is not a necessity, the SASS files are included providing "added value". As you can see, the CSS is merely a few lines and whoever wants to change those is not dependant on SASS. However, defining which workflow is suiting and wether SASS is appropriate or not is clearly depending on personal opinion. If it makes anyones sleep better, I simply remove the SASS files from the project and keep them to myself, but really, I don't see the point.

Thanks for reviewing, and please let me know about the proposed inclusion of the library.

dman’s picture

dman commented

If you replace the included code with instructions to include it as a library, this can be put up for review again.
It can be said that SASS or not is a choice. I feel that adding it as a requirement to *this* module dilutes its purpose for little gain. I'll leave that judgement open for a second opinion.

I found some elements of the code and what it does to the text a little clunky, it was a little hard to follow, and token-based chopping up of HTML always hurts.
But for Drupal Code Standards review, if you sort out the licenced code issue, someone else can take a look at it and make a call then.

Anonymous’s picture

Anonymous (not verified) commented

I will rearrange things accordingly. Thank you for your time.

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.