Hi there,

love the module, but I had some issues grocking where the menu entries were and what is needed to enable this module.

See #1.

CommentFileSizeAuthor
#8 1734604-8.patch10.84 KBkiphaas7
#7 1734604-6.patch10.36 KBkiphaas7
#3 patch_commit_1452c1f6ca2a.patch10.48 KBkiphaas7
#1 patch_commit_766b3ec10921.patch10.46 KBkiphaas7

Comments

kiphaas7’s picture

kiphaas7 commented
Status: Active » Needs review
StatusFileSize
new patch_commit_766b3ec10921.patch10.46 KB

Attached patch tries to:

  • Fix some very minor typo's in the inline documentation
  • Change around the menu entries to be local tasks (so they appear together)
  • Expand explanation of the "enabled state" of the module, adding that it does not work if the site is either in maintenance mode or if "vanilla aggregation" is not enabled: especially the last one was a huge WTF for me, I assumed this module would take over aggregation and thus I'd actually need to disable vanilla aggregation. Something that only cleared up as soon as I looked in the source code
  • Group admin settings per subject (in 3 fieldsets)

Pretty straightforward patch, though looking forward to any critique on it!

nagba’s picture

nagba commented

Thanks, I will take a look at it in the coming days. Better explanation is always welcomed, a fresh set of eyes helps a lot. :)

kiphaas7’s picture

kiphaas7 commented
StatusFileSize
new patch_commit_1452c1f6ca2a.patch10.48 KB

Actually missed a t() call in an addition, updated patch.

Glad to be of any help :).

kiphaas7’s picture

kiphaas7 commented

By the way: if the patch is too big to review at once, I actually made all the changes in my sandbox:
http://drupal.org/node/1732740/commits

Separates the above patch in 5 commits.

kiphaas7’s picture

kiphaas7 commented
Status: Needs review » Needs work

+ '!aggregate' => l(t('aggregate CSS and/or JS files'), 'admin/config/development/performance', array('attributes' => array('title' => t('Enable or disable page caching for anonymous users and set CSS and JS bandwidth optimization options.')))),

Has a trailing whitespace :/. Will fix later today.

kiphaas7’s picture

kiphaas7 commented
Status: Needs work » Needs review
StatusFileSize
new 1734604-6.patch10.36 KB
kiphaas7’s picture

kiphaas7 commented
StatusFileSize
new 1734604-8.patch10.84 KB

This is starting to look like spamming, but I actually honestly forgot to update the .info file...

nagba’s picture

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

looks good to me

nagba’s picture

nagba commented
Status: Reviewed & tested by the community » Fixed

I've commited your code, hopefully soon the dev release will update itself and i'll be able to create a new prod release as well.

kiphaas7’s picture

kiphaas7 commented

thanks!

nagba’s picture

nagba commented
Status: Fixed » Closed (fixed)

Closing issue as the code is now on both dev and stable releases.

nagba’s picture

nagba commented
Issue summary: View changes

clarify OP