The Message Toggle module adds SHOW/HIDE links to all Drupal messages. Clicking these links within a message will toggle the visibility of that message. It is powered by jQuery and there is no configuration. Keeping it simple.

The module was developed out of the need to toggle the visibility of messages rather than hide them (see Dismiss). This was not provided by the other, often more complex, message-related modules.

Message Toggle sandbox.

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/guschilds/1961192.git message_toggle

Comments

SamChez’s picture

SamChez commented
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

FILE: /var/www/drupal-7-pareview/pareview_temp/css/message_toggle.css
--------------------------------------------------------------------------------
FOUND 11 ERROR(S) AFFECTING 11 LINE(S)
--------------------------------------------------------------------------------
18 | ERROR | CSS colours must be defined in lowercase; expected #4897b9 but
| | found #4897B9
40 | ERROR | CSS colours must be defined in lowercase; expected #fff but found
| | #FFF
44 | ERROR | CSS colours must be defined in lowercase; expected #be7 but found
| | #BE7
48 | ERROR | CSS colours must be defined in lowercase; expected #be7 but found
| | #BE7
49 | ERROR | CSS colours must be defined in lowercase; expected #f8fff0 but
| | found #F8FFF0
53 | ERROR | CSS colours must be defined in lowercase; expected #ed5 but found
| | #ED5
57 | ERROR | CSS colours must be defined in lowercase; expected #ed5 but found
| | #ED5
58 | ERROR | CSS colours must be defined in lowercase; expected #fffce5 but
| | found #FFFCE5
62 | ERROR | CSS colours must be defined in lowercase; expected #ed541d but
| | found #ED541D
66 | ERROR | CSS colours must be defined in lowercase; expected #ed541d but
| | found #ED541D
67 | ERROR | CSS colours must be defined in lowercase; expected #fef5f1 but
| | found #FEF5F1
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/message_toggle.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
5 | ERROR | The third line in the file doc comment must contain a description
| | and must not be indented
--------------------------------------------------------------------------------

Not too much to worry about as far as errors go and after a manual review it seems your commenting was quite thorough. Aesthetically it's quite clean looking and is looking good, personally I think this should have been made sooner (although ideally we don't want to hide errors in the first place).

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/httpgitdrupalorgsandboxguschilds1961192git

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

deviantintegral’s picture

deviantintegral commented

Just wanted to chime in that we're using this on a large site in production and it's working beautifully. Once these minor style issues are cleaned up I don't see a reason to approve this.

guschilds’s picture

guschilds commented

Thanks for the reviews, SamChez.

I've corrected the errors reported by PAReview.sh. See http://ventral.org/pareview/httpgitdrupalorgsandboxguschilds1961192git

Because of the first sentence, I'm assuming deviantintegral meant to say "I don't see a reason to not approve this"...?

guschilds’s picture

guschilds commented
Status: Needs work » Needs review

Forgot to change the status. See previous comment.

c_lehel’s picture

c_lehel commented

@guschilds

Nice little module. Works fine for me.
An idea for future improvement: once I hide a message, the next message shows expanded. You should save the state (hidden/shown), and display the messages after that point in the desired state (expanded/rolled up). But I repeat, this is more of an idea/future feature request.

pagolo’s picture

pagolo commented

The module works fine, however it can be improved by localizing the HIDE and SHOW strings.
For this purpose see Drupal.t(), the javascript counterpart of php t() function.

guschilds’s picture

guschilds commented

c_lehel, I've definitely considered adding that. I'd love to hear what others think. Will definitely keep it on the radar.

pagolo, Great idea. I've implemented and committed the suggestion.

Thanks to everyone for the feedback thus far!

pranit84’s picture

pranit84
Primary language Hindi
Location Faridabad (India)
commented

Module works fine. but while doing the manual review i found their is no code in .module file. So why you added this file? Even without this .module file the functionality will work.

bulat’s picture

bulat commented

I would rather have a custom theme_status_messages() implementation or some other way of theming messages first instead of building HTML with jQuery.

federiko_’s picture

federiko_ commented

Nice simple module. Works fine for me.

delta’s picture

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

Manual review:
This module works for me, just make sure it's very clear on the projet page that this module works ONLY with javascript enabled. I didn't see any note about that in the README file.
That's all, i found no major flaws.

Thanks

guschilds’s picture

guschilds commented

Many thanks to everyone who has reviewed this!

To address the latest feedback:

  • re #9: Disable the module, remove the .module file, and clear cache. The module will no longer be listed on the Modules page for possible enabling. This is why that file exists.
  • re #10: I did not want to conflict with existing implementations/alterations of theme_status_messages() (I was already overriding it on the project this was created for). Also, perhaps more importantly, I did not want to add markup that wouldn't make sense or function if the user does not have JS enabled. This technique ensures messages will not be affected in any way should JS be disabled.
  • re #12: Thanks. I've updated the project page and committed to README.txt with more explicit mentions of a JS dependency.

Gus

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Status: Reviewed & tested by the community » Needs work

It's an interesting idea, can you flesh it out a little more? I see you listed a comparison node at https://groups.drupal.org/node/51088, can you add your module and tell us how it's different? Currently, this is not enough code to approve you as a git vetted user, however we can still promote the project itself if you'd rather do that.

Code too short
This project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of code or less than 5 functions cannot be seriously reviewed. However, we can promote this single project manually to a full project for you.

----
Top Shelf Modules - Crafted, Curated, Contributed.

guschilds’s picture

guschilds commented
Status: Needs work » Needs review

kscheirer,

Thanks for having a look! I understand your concerns with promoting me as a git vetted user after having seen such little code. I did notice that guideline and wondered if it would play against me here.

This project is different than the others in that comparison node because it simply provides a SHOW/HIDE toggle effect to messages, while the others provide similar, but never the same, effects. I'd be happy to add it there should it get promoted to a full project. When I wrote it I only needed to solve that small need and don't really have any plans to add much more to it unless a reasonable request comes about in the issue queue.

I figure it's worth asking if work elsewhere can be considered when granting git rights. I've already spent a decent amount of time providing support in the issue queue for the YouTube module that I co-maintain with jenlampton. I've also contributed to the recently approved Webform Hints module. If that doesn't matter, I can respect that.

I'd be happy to see this promoted as a project regardless! Thanks again.

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Assigned: Unassigned » kscheirer

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

kscheirer’s picture

kscheirer
Primary language English
Location Vallejo
commented
Assigned: kscheirer » klausi
Status: Needs review » Needs work

Thanks for your response guschilds!

Regarding the not enough code issue - I think the easiest solution is to add some more bits to the module. Implementing a hook_help() or adding simpletests is an easy way to show more lines. Making the show/hide text configurable would allow you to have an admin page with a small form and validate/submit handlers. If you use variable_get() and set the defaults to "Show" / "Hide", then your module still requires 0 configuration and will work just the same.

There are a lot of other modules that theme drupal messages, but they usually do a lot more like formatting is as Growl or some other more complicated implementation. I think your module is nice for being so simple, but if klausi has time I'd like to ask for a second opinion. I'm also not sure whether we can use your other code to evaluate your application.

Setting to "needs work" for the code being too short and assigning to klausi for a second opinion.

----
Top Shelf Modules - Crafted, Curated, Contributed.

klausi’s picture

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

Not enough code is not a problem, and I would advise against artificially blowing up the size of a module. We can always promote short projects manually, and applicants can always come back to us with new projects for approval.

Please add the reason for yet another module of this kind to the project page and specify the exact difference to existing modules so that users can make better decisions which module to use.

Thanks for your contribution, guschilds!

I promoted this project for you: https://drupal.org/project/message_toggle

Now that this experimental project has been promoted, you'll need to update the URL of your remote repository or reclone it.

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!

Thanks, 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 to the dedicated reviewer(s) as well.

guschilds’s picture

guschilds commented

Thanks so much, klausi! I've created the 7.x-1.0 release.

As requested, I've emphasized the Drupal message effect comparison node in the project description. I've also added Message Toggle to that node's table.

Thanks to kscheirer and all other reviewers as well.

Status: Fixed » Closed (fixed)

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