Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zymphonies-dev’s picture

Status: Needs work » Needs review
zymphonies-dev’s picture

Status: Needs review » Needs work

HI,

Please add sandbox url and git repository

thanks
shanid kv

Sibiraj PR’s picture

Status: Needs review » Needs work

hi,
I added the sandbox url at the bottom of the image. Please check.

zymphonies-dev’s picture

HI,

Manual review:
1) style is effecting on admin part also
2) menu not displaying
i have created 3 pages but menus are not displaying
( explain in Summary if any module is required )
3) .container style is effecting on footer

.container {
    background-color: #33CC99;
    margin: 0 auto;
    width: 980px;
}

you can write specific style or use different class name
( screenshot attached )

Thanks
Shanid kv

zymphonies-dev’s picture

Sibiraj PR’s picture

Hi shanidkv,

Thank you for your feedback. I changed the things you are mentioned in your previous comment. Please check.

idflood’s picture

Hi,

Nice theme. Here is a quick review (I just looked at the code, no real test here).

  1. You need to create a 7.x-1.x branch. Have a look at the procedure described here http://drupal.org/node/1127732
  2. One trailing whitespace on line 32 of template.php, and some other trailing whitespace in other files.
  3. In template.php, missing comments (File description, comments bout "Implements hook_*()", ...). For instance see http://drupalcode.org/project/omega.git/blob/refs/heads/7.x-4.x:/templat... for exemples of these comments
  4. In template.php -> minimalisum_load_layout: There must be a space before the left parentheses and after the right parentheses. if (true) { ... } else if (false) {...
  5. The indentation is mixed space and tabs in all files. In drupal the indentation is 2 spaces and no tabs.
  6. I don't know if it's required but I find it much more cleaner to have all the .tpl.php files in a "templates" directory.
  7. In general you should have one extra empty lines for each files (in the .info there are 3 of them with the last one having some trailing whitespace)
  8. Why is there an index.html? It may be cleaner to remove it.

edit: Have a look at the coding standards: http://drupal.org/coding-standards
edit2: Here is an automated review: http://ventral.org/pareview/httpgitdrupalorgsandboxsibiraj1825208git
edit3: Changed link for better procedure to remove the master branch

Sibiraj PR’s picture

FileSize
22.92 KB

Hi idflood,

Thank you for your feedback.

Followed all your point also made the changes.

I tested this code using coder module.

coder review

idflood’s picture

Good work sibiraj. Though there are still some coding standard issues http://ventral.org/pareview/httpgitdrupalorgsandboxsibiraj1825208git

edit: The master branch is still accessible. The following should be enough to get rid of it:

git checkout 7.x-1.x
git branch -D master
git push origin :master
Sibiraj PR’s picture

hi idflood,

Done the thing you have mentioned in this message. Now 7.x-1.x branch enabled

zymphonies-dev’s picture

FileSize
52.67 KB

HI,
Manual review:
1) Error in page--front.tpl.php

Undefined variable: primary_nav in include() (line 17 of \sites\all\themes\minimalism\templates\page--front.tpl.php).

2) Please fix footer styling issue (screenshot attached)

thanks
shanidkv

klausi’s picture

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.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

klausi’s picture

Issue summary: View changes

Added the sandbox url