[D7] Semantic UI

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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.

libraries folder: 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.

Please update you project description to have non-maintainer version of GIT Clone.

Adding review bonus

First impressions (not a formal review, as it's Friday night here)

The repository looks surprisingly heavy. Heaps of files, including half a dozen 3rd party libraries included - need to see about each of their licensing conditions etc now. These could be a problem unless whitelisted by the d.o repository hosting licensing conditions.
* It's attractive out of the box. Holds together well. Until you look at the way it handles content or the UI.
* it seems to have added a whole bunch of unwanted unconfigurable social media add-ons to my footer. Putting content on our page is not a theme job, that should be provided as a block, and would have the advantage of being positionable etc.
* spacing/padding in the teal footer looks unrefined - the social widgets were top-aligned and the 'copyright' notice was bottom-aligned, no margin, so looked rough.
* weirdness happened to my teasers, the 'read more' obscured the bottom quarter of my teaser content.
* the mobile/narrow navigation hamburger didn't seem to work at all - it popped up halfway down the left when resizing the screen, but clicking it does nothing. I'm not sure what it's expected to do.
* the forms, like on /admin/content were unusable. Somehow the theme totally hid the checkboxes and disabled the selects.
* A number of things appeared to be applied to the node edit UI - but they just made it strange.

This was done on a minimal site, that had a smattering of content - using a custom content type and some FieldAPI fields added - but wasn't twisted in any special way.

That's from 10 minutes with it, just looking at what it did to a minimal Drupal install.

Maybe if you explained the way it was expected to be used, or what advantage this actually was expected to add it would be easier to review.

I removed the unnecessary third party files.
Remove the social icon from footer.
You need to install jquery_update module.

Will look into other issues that you mentioned.

@dman Thanks for feedback.

added licence info

@Mukeysh, let me point out a few issues I encountered in your template.php:

1) Not all your hooks and theme implementations in template.php have the "Implements hook_NAME()" or "Overrides theme_NAME()" comment above them. Please add these as per drupal standards.

2) Consider removing the extra space(s) at the end of L3 and on L57 in template.php. As per https://drupal.org/coding-standards, "Lines should have no trailing whitespace at the end."

3) Please remove the extra newline (\n) at the end in template.php. As coding standards, "All text files should end in a SINGLE newline (\n).

@Vinay Punyamurthy these changes has been done.

Hi Mukeysh,

First of all, thank you for contributing this, I like Semantic UI framework!
I did some usability tests and here are some issues that need to be addressed:

- Vertical tabs displays in admin forms are broken, which is a major usabilty problem (see attachment)

- Table with checkboxes (tableselect) that list people, content, etc ... are broken as well. Checkboxes don't display at all.

- Minor: the drag'n drop icon is a little too small to my taste, and makes the action less obvious

Other issues:
- You should probably integrate third-party code with Libraries API module, as mentionned above
- The clone command in the PA summary is still wrong. Use this one instead: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/culfin/2152101.git

Keep up!

@manumilou - Vertical tabs and Table with checkboxes issues has been resolved. Please check.

Thanks for your feedback.

@Mukeysh

I confirm that the vertical tabs and the tableselect are now fixed, nice work.

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxculfin2152101git

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

Fixed function prefix with theme name according to
http://pareview.sh/pareview/httpgitdrupalorgsandboxculfin2152101git

Hi,
Thanks for your contribution.

here is my review

indentations in template.php, style.js and some selectors in responsive.css are 8 spaces. it should only be 2 according to Drupal recommandations https://drupal.org/coding-standards#indenting

there is a type error in semanticUI_html_head_alter documentation ('heaf' instead of 'head')

Hooks documentation should contain '().' after function name, (per example 'Implements hook_css_alter.' should be 'Implements hook_css_alter().' See https://drupal.org/coding-standards/docs#functions

README.txt should not contain lines with more the 80 characters.

Some template files have the '@file' header but some don't (block.preprocess.inc per example).

Some template files documentation are false, per exemple 'Default theme implementation to display a block.' but it is actually an overrite of the default one.

Thanks again for contributing this new Drupal theme.

Automated Review
----------------

OK.

PAR Checklist
-------------

Normalize.css is included, but it is MIT. Other themes include this (like Omega). OK. Per https://drupal.org/node/422996, though MIT is considered GPL-compatible, it is not allowed and needs to be removed.

Else is OK, unless mentioned below.

Code Walkthrough
----------------

As dependencies aren't supported in themes yet (https://drupal.org/node/474684), I would add them commented out to the .info.

SASS for the CSS would be nice.

(*) The JS doesn't follow Drupal best practices (eg, no behaviors). See https://drupal.org/node/751744

(*) semanticUI_preprocess_block should add each class individually. Also elsewhere.

The file is called block.preprocess.inc, but you also have the process function there? This is confusing.

(*) semanticUI_breadcrumb() should call check_plain() instead of strip_tags().

(*) Per https://drupal.org/requirements, the minimum version for Drupal 7.x is PHP 5.2.5. semanticUI_preprocess_field
has an anonymous function, which were introduce in PHP 5.3.

(*) Use drupal_html_class() and drupal_html_id() instead of your own scheme for cleaning up classes (eg, in semanticUI_form_element()).

(*) filter_xss_admin() in semanticUI_form_element_label() is too permissive. check_plain() it.

(*) Why are polyfills handled that was in semanticUI_preprocess_html()? hook_page_alter() is probably a better place, but check how other themes (like Omega) handle this. No documentation about the polyfills?

semanticUI_preprocess_page(), use user_is_anonymous instead of poking around in $user.

semanticUI_preprocess_page() has some untranslated strings.

Commented out code should be removed for production.

The logic in forum-icon.tpl.php should be moved to a preprocess. Also elsewhere.

You have hardcoded metas in html.tpl.php

semanticUI_html_head_alter() adds the viewport, but you have this hardcoded in html.tpl.php

Why the explicit content type in semanticUI_html_head_alter?

The starred items (*) are fairly big API issues, as they will hamper anyone who wants to subtheme this. These warrant going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

.js:

      $('img').each(function() {
        $(this).removeAttr('width')
        $(this).removeAttr('height');
      });

Drupal.behaviors.semantic = {
    attach: function() {
...
    }
  }

These could all use a semicolon;

(*) Naming the project semantic_ui, then calling the theme semanticUI is problematic for subtheming. It isn't clear to a subthemer what they should list as the base theme. And the standard for projects/themes is to use underscores rather than camel case for the machine name. Please rename the .info file and other places. The friendly name (semanticUI) can stay the same.

require_once dirname(__FILE__) . '/preprocess/block.preprocess.inc';
require_once dirname(__FILE__) . '/preprocess/html.preprocess.inc';

module_load_include should be used.

The starred item (*) is a fairly big API issue, as it will hamper anyone who wants to subtheme this. It warrants going back to Needs Review. The rest of the comments in the code walkthrough are recommendations.

module_load_include() shouldn't be used at the top level per the API docs, so require_once is acceptable. The alternate is to use hook_theme_registry_alter() to add them.

Thanks for reviewing the theme. This helps a lot to push the code forward and make it better :-)

I tried to use the word semanticUI instead of semantic_ui because I find it more compact and readable in longer function names like semanticUI_html_head_alter. So if it's only a recommendation not to use camelCase when naming projects, i would be happy if I could leave it as it is. But if it's against Drupals coding standards, I would definitely change it. In this case, could you please post a link where I can find this explicit naming convention?
In the meantime I tried to update the complete code to use semanticUI. Please tell me if I missed something (for example I'm not sure where the theme was named semantic_ui, as you mentioned).

For the other things like JS optimization and using hook_theme_registry_alter() to load the inc files, I hope aprogs will look into this.

Please note that in the latest dev version the folder in which the Semantic UI should be placed should be named "semanticUI" (was "semantic" before). This was changedto have a proper naming consistency in the complete theme.

I would have to find some example code for doing this. The Omega base theme does it. The advantage it that is can lower the memory footprint for page request (if a preprocess isn't used for a page, then the code won't be loaded).

Took a look at the starred issues that I mentioned above.

(*) Per https://drupal.org/node/422996, you need to remove normalize.css. Line 1 is a MIT license header.

Your JS behavior should use the context that is passed in. This is important for AJAX. Also use .once() where appropriate.

semanticUI_breadcrumb(), you don't need the strip_tags(). check_plain is fine.

The elements that you are making in semanticUI_preprocess_html() would normally be done in hook_html_head_alter().

Breadcrumb. I poked around a little. Your usage is OK. See template_preprocess_html() and template_process_page() for two places where drupal_get_title() is used.

Thanks for removing normalize.css. The licensing issue is a big deal.

(*) Name. I cannot point you to a definitive reference, but camel case is not used except for files where the filename is the same as the class name. There may be exceptions to this, but consider it a defacto standard. The friendly name can stay semanticUI. You need to make the choice between semantic_ui and semanticui for your project namespace. Either is fine. Once you take care of this, I can give you a prompt, full review again, and it looks like you are close to RTBC.

In the latest commit I changed all naming to semanticui. The theme name in the info file is now SemanticUI, all other references have been renamed.
I hope I did it right. :-)

Review of the 7.x-1.x branch:

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

  FILE: /home/klausi/pareview_temp/js/style.js
  --------------------------------------------------------------------------------
  FOUND 11 ERRORS AFFECTING 11 LINES
  --------------------------------------------------------------------------------
     1 | ERROR | Missing file doc comment
    29 | ERROR | Inline comments must start with a capital letter
    32 | ERROR | Inline comments must start with a capital letter
    40 | ERROR | Inline comments must start with a capital letter
    45 | ERROR | Inline comments must start with a capital letter
    49 | ERROR | Inline comments must start with a capital letter
    52 | ERROR | Inline comments must start with a capital letter
    60 | ERROR | Inline comments must start with a capital letter
    68 | ERROR | Inline comments must start with a capital letter
    75 | ERROR | Inline comments must start with a capital letter
   124 | ERROR | Inline comments must start with a capital letter
  --------------------------------------------------------------------------------
  

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.

manual review:

1. oh, this is a theme! From the project page it sounded like a module that would enhance the UI elements somehow. Please add to the project page that this is a theme.
2. It seems that culfin opened this project application, but has no commits on the git repository? I would propose that we give the git vetted user role to aprogs, since she/he seems to be the recently active one maintainer? I'll change the application author to aprogs, let me know if this is a mistake.

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.

Assigning to mpdonadio as he might have time to take a final look at this.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2211003

Project 2: https://drupal.org/node/2274889

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Whoops, the author change of the application triggered this. Let's continue here!

This is next up in my queue of reviews to do. It will either be later tonight or tomorrow mid-day (I'm EDT).

When I do a git log on a fresh clone, I am seeing two recent users: culfin and aprogs. Please confirm who would get the vetted user status.

Automated Review

Looks like false positives.

Manual Review

Fill out the README a little more to provide info about how to install the semantic UI library, and the other libraries. Do the same for the project page.

Remove the console.log() from the JS.

The text replacement in semanticui_preprocess_search_block_form() will be a little fragile in the long run. It may also end up catching
things it shouldn't.

forum-topic-list.tpl.php has a filter_xss() in it. I would move this to a preprocess.

Will html.tpl.php work with the RDF module?

After looking at the .tpl.php files, there are a lot of

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2211003

Project 2: https://www.drupal.org/node/2274889

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Sorry, looks like I forgot to escape that. There are a lot of <div> elements being used. I would see if you can work in some of the semantic HTML5 elements where appropriate.

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

Thanks for your contribution, aprogs and other contributors!

I updated your account so you can 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 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.

Unassigning myself.