Project Description

This module hides messages set by drupal_set_message() after a certain time. A progress bar shows when a message will be hidden. When hovering over the message the progress will be paused. Once hidden the corresponding icon is left in place, when clicking the icon, the hidden message reappears. So if you miss a message, you'll still be able to read it.

Screenshot

timed message with progress bar half way through
On the picture you can see the progress bar half way through.
As drupal won't let me show the screenshot, please got to the image itself

Project Page

timed messages sandbox

Git Clone

git clone --branch 7.x-1.x git.drupal.org:sandbox/bechtold/1933482.git

My reviews so far

http://drupal.org/node/1965468#comment-7278916
http://drupal.org/node/1949352#comment-7279016
http://drupal.org/node/1965800#comment-7279240

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

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

arun ak’s picture

arun ak commented

Hi,

You need to follow the drupal coding standard.

In your js file script.js, there is a code : console.log('test'); at line 69.

Please remove such codes that you used for development purpose.

bechtold’s picture

bechtold commented
Status: Needs work » Needs review

I did not know about ventral.org, but it is running through without any errors now.
The only thing Coder still tells me is a missing @file block in the js file, even though it is there. I don't understand what the problem is.

I know about the review bonus program, but so far I didn't find a project I was able to contribute to the review process.

I think its a good thing to review, since I learned a lot about coding guidelines, that I didn't know about.
I will tell my colleagues about this. I'm sure together we will contribute to the review process.

luco’s picture

luco commented
Status: Needs review » Needs work

hi @bechtold,

I installed the module before the library to check how your module handles this case. the message reads, "The <em class="placeholder">Pause (A jQuery plugin to pause and resume animations)</em> library could not be found."

I believe it's because of check_plain() in line 23 of the .module file. I guess you should use t() instead, but I'm unsure of security implications.

the comment block in the .module file has a copyright message in it - again, I'm unsure how to advise you, but since it's open source, I don't think you should copyright that.

the indentation on your sass is inconsistent. that's not very sassy ;)

last but not least, I have a usability-driven feature request:
you could give users the option between progressbar or clock-style ticker. that's because users' eyes already traverse the message horizontally, and it's a little weird reading it with an animated bar right underneath.

other than that it's a neat module and works as expected. hope it's official soon! :)

cheers,
Luciano

bechtold’s picture

bechtold commented
Status: Needs work » Needs review

I believe it's because of check_plain() in line 23 of the .module file. I guess you should use t() instead, but I'm unsure of security implications.

It was the check_plain, that was code I copied from the libraries documentation. I removed the check_plain, but i don't think t() would help here, since its dynamic content. I would assume, that the libraries module uses t() for their error messages.

the comment block in the .module file has a copyright message in it

Right you are, I removed it, thanks for the hint. I always get confused, with the german copyright law :-)

the indentation on your sass is inconsistent.

True, I use netbeans and when I hit "Format" it works perfectly fine on php and stuff, but sadly not on scss. I must have applied the auto format by accident. But I fixed it by hand now. If anyone has a solution for this, please let me know.

option between progressbar or clock-style ticker

This is an awesome idea. I added it to the planned feature list. Thanks for that.

hope it's official soon!

Thanks, me too :-)

beljaako’s picture

beljaako commented

Nice module!

Just some things I ran into:
- It doesn't work with all themes. It seems your looking for a specific class in error messages. It would be nice to make this configurable so one can choose these classes.
- On every page refresh, the same error message appears for x seconds. This can get quite annoying after a while. As you are stating

I am annoyed by messages on my page when I don't need them anymore.

it would be nice not to repeat messages you've allready shown (at least for a while).

bechtold’s picture

bechtold commented

It doesn't work with all themes. It seems your looking for a specific class in error messages. It would be nice to make this configurable so one can choose these classes.

Happens when themes bring their own template for messages I guess. I add this to the feature list. Once I finish the admin page, I will integrate this. Can you tell me which theme it doesn't work with, so I can have a look.

On every page refresh, the same error message appears for x seconds. This can get quite annoying after a while.

Not sure which message you mean, the one about the missing library? Thats why I have in the planned features list, that I want to move the message to the admin page, so it only appears there.

Hroudtwolf’s picture

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

Looks nice.
But, for the future... It is not necessary to use so many db variables.

Thanks for the idea and the module.

bechtold’s picture

bechtold commented

Thanks for reviewing.
In the last commits I've added the admin page and made the pause library optional.
Now the messages about the pause library only appears on the admin page.
You can also enter the classes to select the messages on the admin page.
I've implemented the option on the admin page to not hide a message at all, which might come in handy for error messages.
As Hroudtwolf stated I implemented the variables as tree container to prevent too many database request.
Also to improve performance i've removed unnecessary variables from the hooks, as Hroudwolf suggested (not in this issue queue).

The only thing pareview and coder are complaining about is the missing @file in the .js file, which actually is there and probably the indentation for arrays.
That comes from my IDE which indents more than "allowed" but I have to say, that I find it more readable the way Netbeans does it.
Other than that I find it kind of amusing that coder warns about missing spaces and full stops in comments, but I've added them :-)

I'm glad you guys like my module.

bechtold’s picture

bechtold commented

I found out how to configure Netbeans correctly. For anyboby wondering: http://drupal.org/node/1019816.
I also fixed a little issue with the default value for the messages class and improved the css a little bit.

bechtold’s picture

bechtold commented
Issue summary: View changes

tried to get the image working

bechtold’s picture

bechtold commented
Issue summary: View changes

added first review of other project application

bechtold’s picture

bechtold commented
Issue summary: View changes

added second review of other application

bechtold’s picture

bechtold commented

I have reviewed some applications now.
Its kind of fun, as you dig into drupal stuff you wouldn't without reviewing :-)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Fixed

Sorry for the delay, you forgot to add the PAReview: review bonus tag as described in http://drupal.org/node/1975228

manual review:

  1. "variable_get('timed_messages');": all variables defined by your module need to be removed in hook_uninstall().
  2. "'access arguments' => array('administer time messages configuration'),": that permission is not defined with hook_permission()?
  3. timed_messages_menu(): access arguments are duplicated.
  4. @author tags are not usually used in Drupal, because over time multiple contributors will touch the code anyway.

Although you should definitively fix those issues I don't think they are critical application blockers, so ...

Thanks for your contribution, bechtold!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

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

bechtold’s picture

bechtold commented

Hey,
thanks klausi.
I fixed the stuff you mentioned.

I promoted it and it is available over at http://drupal.org/project/timed_messages.

Cheers
bechtold

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

added third review