[D8] Blocky

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

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.

Automated Review

In this case, the automated review findings are not terribly useful, since the errors reported are for files that are copied directly from core.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

Solid profile, Christopher!

No duplication

No: Causes theme duplication and/or fragmentation.

I think this is a great concept for theme. Twig blocks rock, and it's great to see a cool demonstration for how to make use of Twig blocks. That said, I don't see why this theme needs to be a duplication of the Stable theme. You're copying all of the CSS and images in Stable, and then overriding core with your versions of those files, but since I don't think you're doing anything different with the CSS and images, that seems like a lot of unnecessary duplication. If I missed something, and there's a good reason for that, glad to know.

However it seems like a simpler solution would be to set Stable as the base theme for Blocky, and then override only the templates where you want to demonstrate Twig blocks. That would make it much more clear what your particular theme is doing versus what Stable does in core in terms of providing a BC layer.

Master Branch

Yes: Follows the guidelines for master branch.

8.x-1.x: check!

Licensing

Yes: Follows the licensing requirements.

I don't see any third-party code or licensing problems with the theme.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

No third-party assets or code. All good here.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

This seems generally okay. The project page lists which templates now have Twig blocks, which is handy and could be useful in the README. That wouldn't be necessary if Blocky extended Stable and the theme only had the templates that now used Twig blocks. The Twig blocks in the templates don't have code comments, but they're pretty self-explanatory given the block names. Not sure that adding code comments for those would be particularly helpful. So this seems pretty solid.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

In reality the unique code for this is the templates with Twig blocks. Everything else is copied and modified as needed from Stable. Those templates do have a sufficient amount of code to review.

Secure code

Yes: Meets the security requirements.

The templates are really the primary code for review here, and I don't see anything in the Twig files that looks like an issue.

Coding style & Drupal API usage

I don't really see any coding issues, per se. But I do think this project would be vastly improved and much easier to manually review if the theme extended Stable. If there's a really, really good reason not to do that, that's worth considering. But this is going to be much easier to maintain and understand if it isn't duplicating so much of Stable.

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

This review uses the Project Application Review Template.

Final note: Interesting theme! And I do think it is worth considering this for core as well. Hope you can trim this theme down to its essence, as it's a very interesting demo of the power of Twig blocks.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause theme duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity. This theme is obviously very simple and probably does not meet the "complexity" guidelines of a module, however I feel like the intent has potential and deserves to be a full project. That being said, are there more templates from Stable that could be overriden in Blocky?

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:

1. I'd recommend adding a simple composer.json file to define this project as a "theme". This is esp. helpful for people using things like the drupal-composer/drupal-project
2. Add a default Blocky logo? I'm getting a 404: Logo not found /themes/contrib/blocky/logo.svg
3. There's a small typo in the project page documentation, the example code block should extend "node.html.twig" not "node.twig.html"
4. I realize this is an incredibly minimal project, but since it's meant to be used as a base theme, I think it'd be better with a starterkit. The starterkit might even include some example overriden templates, using extends (or embed or include or whatever, as discussed in your post)

This review uses the Project Application Review Template.

Hope that's helpful.

You create a following files in your theme:-
html.twig.html
page.twig.html
node.twig.html
block.twig.html
comment.twig.html
search_result.twig.html

I think you forget to create block.liberaries.yml file.This file is necessary in D8 theme.

As part of Project application checklist - https://www.drupal.org/node/1587704

It says - under section 4. Licensing checks
4.1 Ensure the top directory of the repository does not contain a ‘LICENSE.txt’ (or similar) file.
"Do not include a LICENSE.txt file in your repository. Drupal.org will add the appropriate version automatically during packaging."

Please remove it from your repository.

The license file says GPLv2+, so that is the same as on drupal.org. Removing the license file is surely not an application blocker, anything else that you found or should this be RTBC instead?

looks RTBC to me then..

Thanks for your contribution, chris_hall_hu_cheng! You are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to 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 (here you get to ask fans of your project to help you!)

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, especially my slowness in approving following review. We know it's broken and are trying to fix it.