Pretty Calendar

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
(At least this has to be done before switching back to needs review)

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxdmitriymakeev1488326git

You can also get a review bonus and we will come back to your application sooner.
:)

Hi,

I've tested your module, and here are my comments:

1. I'd recommend a README.txt file which explains how to set it up. I didn't realise at first that i had to "Configure" the block to select theme and node types to display in the calendar.

2. The section "Event Headings" isn't quite clear to me. Is this a way of providing singular and plural form of content name? If so, could it simply be two text fields; one for singular and one for plural?

3. Maybe you could add two radio buttons instead of the list under "Tooltip Type". Then the first radio button could say something along the lines of "Display summary of nodes by day" and "Display links to nodes".

I thing your module appears to be easy to set up and use. A suggestion might be to integrate it with views so that one can create a view which the calendar should use as its data source. Right now, it just displays all nodes (or the content types on selects)...but it would be great to be able to narrow it down to nodes provided by a view...or alternatively nodes in a taxonomy.

Last comment (apologise if this might be a little subjective)...but the name of the module does appear a little...well...kinky. At first i thought it would have something to do with "adult content". I'm not telling you to change the name...just letting you know that even though it seems to be a good module people might think that it is something very different...:-D

Note! Haven't done any coding review. Just looked at the actual functionality.

You still have files other than the README in the master branch. Please read this page: http://drupal.org/node/1127732

In the module file:

In functions russian_beauty_calendar_content_loader() and russian_beauty_calendar_links_loader(), don't use exit(), use drupal_exit() instead.
http://api.drupal.org/api/drupal/includes!common.inc/function/drupal_exit/7

// thanks for removing that

Hi Dmitriy!
First, gotta tell you, love the name. And the guts you have for working with dates.

Also, just wondering do you plan to integrate this with date.module at all?

Here's the rest -

http://drupalcode.org/sandbox/DmitriyMakeev/1488326.git/blob/refs/heads/...

Very hard to read. Look at keeping the lines around 80 characters long

http://drupalcode.org/sandbox/DmitriyMakeev/1488326.git/blob/refs/heads/...

Got a comment about this on my last review, and it's a good practice so thought I'd pass it on:

Keep your variables list in a centralized function. That way when you uninstall,
you just get rid of the variables that you defined. So for example if I make a module called russian_beauty_calendar_naughty (I had to, the name is awesome!), an uninstall of this module would remove my variables as well.

Here's the specific comment - http://drupal.org/node/1510938#comment-5830304 and the link it pointed me to - http://drupal.stackexchange.com/a/2476/432

http://drupalcode.org/sandbox/DmitriyMakeev/1488326.git/blob/refs/heads/...

In order to adhere to d.o standards you should document your args going into the function :)
http://drupal.org/node/1354#functions

Agree with @MrMaksimize on hard to read, add more inline comments and keep line lengths shorter.

I have some concerns over your use of while (TRUE) on line 295 of russian_beauty_calendar.module. It looks like $day_count can only be incremented 7 times within the while block but the break only occurs if ($day_count > $dayofmonth). Perhaps there is some potential for adding a safeguard here to prevent an infinite loop?

Hi

Have you never thought to integrate your module with view to make it more dynamic ?

Cheers,

I was thinking of something like http://drupal.org/project/fullcalendar

I don't mean to discourage you, but someone has to ask:
How does this differ from the Calendar module?

Could you point out what it was that made you create your own version and what the main differences are?

All right, perhaps one more remark.

See #1535540-14: Hooker hunter

I know nit-picking over things like names seems tedious, but it's important to be culturally sensitive especially given how diverse Drupal's user base is.

Admitted that that guy's module name was hilariously wrong, I can't help but expect a bunch of 20-year-old Russian blondes wearing little more than a thong when I click on a link titled "Russian Beauty Calendar".

Seeing as your module has some very pretty looking calendars, why not rename it to something that reflects that. Like "Pretty calendar"?

I can't help but expect a bunch of 20-year-old Russian blondes wearing little more than a thong when I click on a link titled "Russian Beauty Calendar".

Same here ;-)

I'd also agree changing the name to something like "Pretty calendar", that would really outline clearly what it does.

After doing a manual code review, I came up with this:

hook_menu
'access callback' => 'user_access', can be omitted as user_access is the default.

hook_install
You don't really need to set all your variables here, as long as you always provide the second parameter for variable_get(). The upside of setting them is that you don't need to provide the default everywhere, the downside is that you can't easily see what the default value is supposed to be.

100 'is_clean_url' => variable_get('clean_url'),
145 '#default_value' => variable_get('pretty_calendar_theme', 'round'),
You seem to switch between providing and omitting a default. Try sticking to one way of doing it.

Obsolete functions?
Are pretty_calendar_year_to_arg, pretty_calendar_month_to_arg and pretty_calendar_day_to_arg used?

149: pretty_calendar_node_type
Multi-selects are still a usability WTF to a lot of people. Seeing as the amount node types is usually limited, perhaps a series of checkboxes could do the trick? This is merely a suggestion, though!

344: Russian month names
Don't hard code translatable items. Instead, add them to the translatable strings as such: t('English month name'). If you want to save Russian users the trouble, you could always provide a module translation alongside your module.

Although I think format_date() handles month names for every language?

405: rbc_* class names
Try to make your CSS class names more meaningful. Especially after the module name change, rbc will look confusing.

457: pretty_calendar_content_loader
Are you sure this is the Drupal way of handling AJAX callbacks? I'm no expert at that, but it seems a bit weird to me.

519: function pretty_calendar_plural
Why not use format_plural()?

Why the wrapper function?

712 /**
713 * Display full calendar.
714 */
715 function pretty_calendar_full() {
716 return pretty_calendar_node_list();
717 }

Your changes look nice, well done!

Two minor and one normal remarks:

• Minor: Your css files show classes that start with .calendar instead of .pretty-calendar, this could cause naming collisions if people have both your module and Calendar activated.
• Minor: Same goes for some paths you reserve in hook_menu. Calendar already uses /calendar, for example.
• Normal: When I asked if your _to_arg() functions were being used, I was actually inquiring whether they do anything useful for your hook_menu wildcards or not. If not, they were indeed obsolete :) If they do, removing them may have broken some functionality. Please verify if removing them was the right thing to do.

See hook_menu()

You can also define a %wildcard_to_arg() function (for the example menu entry above this would be 'mymodule_abc_to_arg()'). The _to_arg() function is invoked to retrieve a value that is used in the path in place of the wildcard. A good example is user.module, which defines user_uid_optional_to_arg() (corresponding to the menu entry 'user/%user_uid_optional'). Etc...

All in all, your code looks great and you seem to use the tools that Drupal has to offer the right way. I'd definitely give you the thumbs up for that. Now, if anyone could review the actual functionality and usability of your work, this could be marked as RTBC for me.

when i try git clone http://git.drupal.org/sandbox/DmitriyMakeev/1488326.git pretty_calendar - all i get is the readme.txt, no actual code.

String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms in line number 2 and line number 3 in pretty_calendar.info file.
description = Simple nice calendar module that displays the materials by date.
core = 7.x

The code looks relatively good ... the module is functioning as expected per the README and project page. The code in pretty-calendar-day.tpl.php has weird formatting though... usually template files should include the PHP tags inline instead of on all of the separate line breaks you have (it looks correct in pretty-calendar.tpl.php) . And you can do some concatenation instead of all of those different print statements in lines 26-29.

Running the Code Sniffer, there are some minor formatting issues:
111 | ERROR | Line indented incorrectly; expected 8 spaces, found 10
252 | ERROR | Array indentation error, expected 4 spaces but found 6
253 | ERROR | Array indentation error, expected 4 spaces but found 6
467 | ERROR | Function comment short description must be on a single line

I've also found a bug. It's minor, but it is a bug that would stop me from using this module. Correct me if i'm wrong but I don't believe this is by design:

• I've set the Calendar to use "Display links to nodes by day."
• When I click on a box inside the calendar with a date on it (with nodes), I am brought to that node and the calendar stays in the Month I had navigated to - this is good
• The bug is when you click on the node link in the tooltip. The node loads but the calendar defaults to the current date, instead of staying on the month I had navigated to.

Additionally, a wishlist item would be a way to customize the "Activities calendar" page title which is hardcoded.

Yes, the file is pretty_calendar.info and it should contain content in following manner:-

"description" .= "Simple nice calendar module that displays the materials by date."
"core" .= "7.x"

Whoa whoa whoa, developers_rtpl: What are you talking about?

A module's .info file is NOT php and should therefore NOT concatenate strings in any way.
The way it is right now is the way it should be!

Please read Writing module .info files (Drupal 7.x) yourself before giving faulty advice.
Misinforming a student during his learning process is one of the worst things you can do :(

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

manual review:

1. pretty_calendar_install(): empty function, so remove it.
2. the script.js files are all the same for all themes?
3. pretty_calendar_links_loader(): why can't you use theme('item_list', ...) to generate the list?
4. "t('more') . ' ' . $links": do not concatenate variables into translatable strings, use placeholders with t() instead.
5. "variable_get('pretty_calendar_page_title', 'Activities calendar')": the default user facing text must run through t() for translation.
6. "@comment": we do not use that annotation in doc blocks, that's what the function long description is for. See http://drupal.org/node/1354#functions

But otherwise looks RTBC to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Still RTBC.

no objections for more than a week, so ...

Thanks for your contribution, DmitriyMakeev!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.

Hi,

I added an end date field for my "Event" content type, is it possible for the calendar to detect that field and show it as a range between the beginning and the end date?

Thanks in advance

Gladstone

Hi gladstoneo

This issue is for the initial project application for the Pretty Calendar module only. If you have any bug reports or feature requests, please add them as new issues on the project's own pages:

http://drupal.org/project/pretty_calendar

Thanks!

how to make Sunday as first day of week currently it is set to Monday

Works nice, except the issue I´ve found:

I´ve installed this module last month and it did what expected from. Now the month has changed and it won´t show up on the block. The actual date is signed correctly, but every time I visit the node it shows up the last month (Nov.), not the actual month(Dec.). A visitor must change the month from Nov to Dec.

How can I change this behavior?

@rabinkmali & tontal

Please look at #40.

Thanks.

Add reviews