[D7] Woodsman - Omega Subtheme [#1978712]

Hi,

Please see below for my submission of my new blog theme "Woodsman". Woodsman is an Omega 3.x subtheme designed to be a personal blog

Project page: http://drupal.org/sandbox/SalahMessaoud/1976446
Git clone: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/SalahMessaoud/1976446.git woodsman
Drupal version: 7, not further dependancies

For my project submission I have followed the istructions at: http://drupal.org/node/1011698 including but not limited to the following steps:

I have performed a coder review at "minor" level. There were two fails and they are explained below in my report.
I have perfomed a pareviewsh report uing the online service at http://ventral.org/pareview and the report is located at http://ventral.org/pareview/httpgitdrupalorgsandboxsalahmessaoud1976446git
- There are some code review failures and they are explained below.

CODER REPORT

SITES/ALL/THEMES/CUSTOM/WOODSMAN/JS/JQUERY.EXAMPLE.MIN.JS
This is an external *GPL licensed* library so I have not touched it.
SITES/ALL/THEMES/CUSTOM/WOODSMAN/JS/WOODSMAN_THEME.JS
It states that the @file block is missing, but I *do* have one there.

PAREVIEWSH REPORT

FILE: /var/www/drupal-7-pareview/pareview_temp/css/README.txt
This is a standard Omega subtheme generated file so I have not touched it.
FILE: /var/www/drupal-7-pareview/pareview_temp/js/jquery.example.min.js
This is an external *GPL licensed* library so I have not touched it.
FILE: /var/www/drupal-7-pareview/pareview_temp/preprocess/README.txt
This is a standard Omega subtheme generated file so I have not touched it.
FILE: /var/www/drupal-7-pareview/pareview_temp/process/README.txt
This is a standard Omega subtheme generated file so I have not touched it.
FILE: /var/www/drupal-7-pareview/pareview_temp/templates/README.txt
This is a standard Omega subtheme generated file so I have not touched it.

The theme has a README.txt file and the contents of that is as follows:

*Installation Instructions*

To install the theme perfom the following steps:

1. Download omega
2. Install Omega
3. Install Woodsman
4. Enable and set default Woodsman

*Developer Instructions*

To initialize the theme from it's raw download state to a state that can be used to develop and improve the theme, perform the following steps:

1. Perform the installation steps above
2. Download modules devel
3. Enable devel
4. Generate content 50 nodes with comments
5. Write a summry for the about page make it promoted / sticky to the top
6. Upload a logo
7. Generate 4 menus
8. Add Main Menu to branding region
9. Hide the image field from teaser display mode
10. Add blocks to sidebar

Comments

Comment #1

salah messaoud commented 24 April 2013 at 14:20

Status:

Active

» Needs review

Comment #2

PA robot commented 24 April 2013 at 16:23

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.

Comment #3

chetan-singhal commented 8 May 2013 at 08:38

Hi @Salah Messaoud

You have many issue regarding space between operators. And line exceed more then 80 line.

FILE: /var/www/drupal-7-pareview/pareview_temp/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
14 | WARNING | Line exceeds 80 characters; contains 143 characters
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/templates/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 13 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | Line exceeds 80 characters; contains 90 characters
2 | WARNING | Line exceeds 80 characters; contains 81 characters
3 | WARNING | Line exceeds 80 characters; contains 88 characters
4 | WARNING | Line exceeds 80 characters; contains 89 characters
5 | WARNING | Line exceeds 80 characters; contains 89 characters
6 | WARNING | Line exceeds 80 characters; contains 89 characters
8 | WARNING | Line exceeds 80 characters; contains 90 characters
10 | WARNING | Line exceeds 80 characters; contains 90 characters
12 | WARNING | Line exceeds 80 characters; contains 90 characters
19 | WARNING | Line exceeds 80 characters; contains 90 characters
21 | WARNING | Line exceeds 80 characters; contains 89 characters
22 | WARNING | Line exceeds 80 characters; contains 88 characters
25 | WARNING | Line exceeds 80 characters; contains 87 characters
--------------------------------------------------------------------------------

First resolved above issues.

Comment #4

lexicon commented 13 May 2013 at 08:59

Status:

Needs review

» Needs work

Hi,

First resolve spacing related issues.
Reduce the no of lines in README.txt file.

Comment #5

salah messaoud commented 13 May 2013 at 10:57

@cpsinghal and @lexicon

As stated in my original application, this these issues come from Omega theme which is a highly reputed base theme so I thought it OK to leave those issues. Nonetheless, I have fixed the README files issues anyway
FILE: /var/www/drupal-7-pareview/pareview_temp/js/jquery.example.min.js

As stated in the original application, this is an external GPL licensed library so I have not touched it.

Comment #6

lexicon commented 15 May 2013 at 04:52

Hi @Salah Messaoud,

You have to make changes in jquery.example.min.js file. Because we have to follow drupal stander. Its' just an spacing problem. Fix it.

Comment #7

salah messaoud commented 15 May 2013 at 10:36

Ok it is done

Comment #8

mayank-kamothi commented 15 May 2013 at 10:37

Hi Salah Messaoud,

Manual Review:
I have check your block--branding.tpl.php file @line no:19 there is one if condition but no any statement is write in the condition if this condition is not in used than remove it.

Thanks,
Mayank

Comment #9

salah messaoud commented 15 May 2013 at 10:50

Hi Mayank,

Thanks for the review I appreciate it, I have cleaned up that code see this commit
http://drupalcode.org/sandbox/SalahMessaoud/1976446.git/commit/c31af13

P.S is there an easy way to reference commit ids on Drupal.org like we can with issue numbers? I couldn't see how

Comment #10

lexicon commented 16 May 2013 at 04:55

HI Salah Messaoud,

When we install your theme according to given instruction in readme. we get the following issue:

Notice: Undefined variable: breadcrumb in include() (line 18 of E:\wamp\www\theme_test\sites\all\themes\woodsman\templates\region--content.tpl.php).
Notice: Undefined variable: breadcrumb in include() (line 18 of E:\wamp\www\theme_test\sites\all\themes\woodsman\templates\region--content.tpl.php).
Notice: Undefined variable: breadcrumb in include() (line 18 of E:\wamp\www\theme_test\sites\all\themes\woodsman\templates\region--content.tpl.php).

Please fix it.

Thanks
Lexicon

Comment #11

salah messaoud commented 16 May 2013 at 13:06

Hi Lexicon,

Thanks for your time and effort, the above issues fixed.

Comment #12

willieseabrook commented 18 May 2013 at 10:59

Status:

Needs work

» Reviewed & tested by the community

All Pareview tests are passing, this has been reviewed by 3 people and in addition I've confirmed it's functional and the code is clean, so setting to reviewed and tested.

Comment #13

willieseabrook commented 31 May 2013 at 12:40

Priority:

Normal

» Major

This has been in review for over a month, and confirmed as good as per comment #12 above, so as per https://drupal.org/node/539608 I am upgrading the priority to major.

Comment #14

willieseabrook commented 13 June 2013 at 11:22

Priority:

Major

» Critical

Deleted...

Comment #15

willieseabrook commented 13 June 2013 at 11:22

Deleted...

Comment #16

willieseabrook commented 13 June 2013 at 11:22

Priority:

Critical

» Major

Deleted...

Comment #17

willieseabrook commented 13 June 2013 at 11:22

Priority:

Major

» Critical

This has been in review for almost 2 months, and ready to be approved for almost a month, so as per https://drupal.org/node/539608 I am upgrading the priority to critical.

We're following the project application procedures as best we understand from reading the docs, so I hope this is not interpreted as being pushy. We appreciate any time it would take for an admin to approve this project!

Comment #18

willieseabrook commented 18 July 2013 at 15:10

Bump.

This has been in review for over 3 months now, and has been ready to be approved for over 2 months. It's been a month since I last bumped this, so I'm bumping it again as per https://drupal.org/node/539608

Comment #19

kscheirer

English

Vallejo

commented 30 July 2013 at 19:32

Title:	[D7] New Omega subtheme Woodsman	» [D7] Woodsman - Omega Subtheme
Priority:	Critical	» Normal
Status:	Reviewed & tested by the community	» Needs work

In woodsman_preprocess_page() image_get_info($logo_path, $toolkit = FALSE); should just be image_get_info($logo_path); since FALSE is already the default value.

Can the if (module_exists('views')) { ... } part of woodsman_preprocess_page() and woodsman_alpha_preprocess_region() be moved to a views specific function? Views has all kinds of hooks and places to override, checking that on every page load seems pretty heavy-handed just to change a title for a single view.

PAReview: 3rd party code: jQuery Form Example Plugin 1.6.0 appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.
The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

I see in woodsman_theme.js you're adding some google fonts, is it possible to use the @font-your-face module instead to do this? I could be wrong, I don't write themes.

Needs a little work, but you're almost there. Ventral.org report comes back clean.

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

Comment #20

salah messaoud commented 1 August 2013 at 14:32

Hi @kscheirer

Thank you for the review

In woodsman_preprocess_page() image_get_info($logo_path, $toolkit = FALSE) ....

This is fixed.

Can the if (module_exists('views')) { ... } part of woodsman_preprocess_page....

This is fixed.

I tried this to check. The views hook that is called at the latest time is hook_views_pre_render and that is called before the page title is set, so it can't be used for my purpose. It could be, but the code would be much more complex. Two function calls module_exists and views_get_page_view are not ideal, but are fast and wont cause any performance issues, and it works. What do you think, is this ok to leave? With the views hook approach not going to work I dont see another way.

PAReview: 3rd party code

This is fixed.

I have removed this third party code and now use the libraries module api instead. This gracefully degrades if the libraries module is not installed or the library is not available.

I see in woodsman_theme.js you're adding some google fonts...

I hope that this is OK like it is. The jquery.example javascript just adds a nice little UX enhancement, and so it doesnt matter if the libraries module is not available and thus this enhancement doesnt work.

However, the custom webfonts are crucial to this theme functionality, and so for the matter of a few lines of (clean) code, I dont want to make this theme dependent on another, quite advanced module.

Comment #21

willieseabrook commented 4 August 2013 at 01:13

Status:

Needs work

» Reviewed & tested by the community

Changing to correct status

Comment #22

kscheirer

English

Vallejo

commented 9 August 2013 at 02:05

Thanks for those fixes, looks good to me!

Comment #23

kscheirer

English

Vallejo

commented 10 August 2013 at 21:23

Status:

Reviewed & tested by the community

» Fixed

Yeah, apparently that's how themes are expected to add something like a google font. Odd that we have a nice way of doing that for modules but not themes (who are far more likely to request a new font).

Thanks for your contribution, Salah Messaoud!

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.

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

Comment #24

drupalfever commented 11 August 2013 at 12:51

Category:	task	» bug
Priority:	Normal	» Major
Status:	Fixed	» Active
Issue tags:		+javascript error

I enabled your theme on my CentOS Linux machine and got a JavaScript error message right out of the box.

I using Firefox 17.0.7 for Linux when opening the page with your theme enabled in my development environment.

The JavaScript error message is as follows:

TypeError: $(...).example is not a function
$('#edit-search-block-form--2').example('Type your search and hit enter...');

Since there was a JavaScript error on the page, the "Grid" button didn't work.

With this error going on, I feel like I have my hands tied and cannot further examine your Theme. I will gladly review your theme again once this problem is fixed.

Comment #25

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 11 August 2013 at 17:33

Category:	bug	» task
Priority:	Major	» Normal
Status:	Active	» Fixed

This project application is closed. Please report all bugs to the theme's issue queue.

Comment #26

salah messaoud commented 12 August 2013 at 13:59

#2063277: Javascript error @drupalfever, I transferred the issue to issue queue

@kscheirer thank you very much for you time reviewing my theme I really appreciate it

Comment #27

26 August 2013 at 14:01

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

[D7] Woodsman - Omega Subtheme

Comments

Comment #1

Comment #2

Comment #3

Comment #4

Comment #5

Comment #6

Comment #7

Comment #8

Comment #9

Comment #10

Comment #11

Comment #12

Comment #13

Comment #14

Comment #15

Comment #16

Comment #17

Comment #18

Comment #19

Comment #20

Comment #21

Comment #22

Comment #23

Comment #24

Comment #25

Comment #26

Comment #27

News items

Our community

Documentation

Drupal code base

Governance of community